-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update workspace dependents on release #101
Conversation
This is a step towards crate-ci#77.
Benefits - Easier to scroll through a lot of tests - Easier to distinguish code under test from condition under test.
This is a step towards crate-ci#77.
@@ -54,6 +55,7 @@ impl Default for Config { | |||
disable_tag: false, | |||
enable_features: vec![], | |||
enable_all_features: false, | |||
dependent_version: Default::default(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of this config flag name but couldn't think of a better one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it needs a better and simple name to understand.
Should we block merging based on the name being less than desirable? If so, how do we overcome this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind. We can keep using this name in beta versions until we come up with a better one.
@@ -363,6 +422,10 @@ struct ReleaseOpt { | |||
/// Do not create git tag | |||
skip_tag: bool, | |||
|
|||
#[structopt(long = "dependent-version", raw(possible_values = "&config::DependentVersion::variants()", case_insensitive = "true"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of this CLI flag name but couldn't come up with a better one
if version.is_prerelease() { | ||
version.pre.clear(); | ||
need_commit = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using default patch
level on pre-release it works exact this way. I suggest not to add another Release
level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was maintaining the existing default which has different logic than patch.
Is your preference just to make patch
the default instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked again. The default behaviour is for the case that when release failed, for example, an error on cargo publish
, the version is already bumped and we have a way to rerun cargo-release
. Otherwise, it makes more sense to use patch
by default.
To fix the rerun issue, also mentioned in #103 , I am going to add a transaction log file like maven release, so we can always rerun the failed process.
Sorry for late review. The |
@@ -54,6 +55,7 @@ impl Default for Config { | |||
disable_tag: false, | |||
enable_features: vec![], | |||
enable_all_features: false, | |||
dependent_version: Default::default(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind. We can keep using this name in beta versions until we come up with a better one.
if version.is_prerelease() { | ||
version.pre.clear(); | ||
need_commit = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked again. The default behaviour is for the case that when release failed, for example, an error on cargo publish
, the version is already bumped and we have a way to rerun cargo-release
. Otherwise, it makes more sense to use patch
by default.
To fix the rerun issue, also mentioned in #103 , I am going to add a transaction log file like maven release, so we can always rerun the failed process.
Fixes #77
This includes switching to CLI arguments using
enum
where possible. The downside toarg_enum!
is that--help
will show the names in uppercase and accepts them case insensitively which is uncommon in CLIs. We probably want to eventually roll our own set ofimpl
s rather than relying onarg_enum!