-
Notifications
You must be signed in to change notification settings - Fork 21
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
53 - Package promotion enhancement #61
base: main
Are you sure you want to change the base?
Conversation
…ady promoted in another build
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #61 +/- ##
==========================================
+ Coverage 46.28% 46.36% +0.07%
==========================================
Files 73 73
Lines 2830 2836 +6
Branches 324 328 +4
==========================================
+ Hits 1310 1315 +5
- Misses 1495 1496 +1
Partials 25 25 ☔ View full report in Codecov by Sentry. |
@@ -27,7 +26,7 @@ export default class PromoteUnlockedPackageImpl { | |||
result.id = packageVersionData.SubscriberPackageVersionId; | |||
} catch (e) { | |||
if (e.message.includes('previously released')) { | |||
SFPLogger.log(`Package ${this.package_version_id} is already promoted, Ignoring`); | |||
throw new Error(`The package version ${packageVersionData.MajorVersion}.${packageVersionData.MinorVersion}.${packageVersionData.PatchVersion} was already promoted in a previous build. For a given this version number, you can promote only one version.`); |
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.
@dieffrei There is an issue with the approach, if you attempt do rerun an existing release, this will cause failures
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.
@azlam-abdulsalam what do you think if we add a new parameter, "--fail-when-is-promoted"?
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.
Yeah, it makes sense. Lets add it, and also ensure release command remains compatible
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.
@azlam-abdulsalam perhaps it was different in the past, but if you attempt to promote package which is already promoted the CLI does not throw an error. Instead it's a success and this message:
Successfully promoted the package version, ID: 04txxxxxxx
|
||
export default class PromoteUnlockedPackageImpl { | ||
public constructor( | ||
private project_directory: string, | ||
private package_version_id: string, | ||
private devhub_alias: string | ||
private devhub_alias: string, | ||
private fail_if_is_alreadyPromoted: boolean |
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.
Why variable name in the constructor have this different naming convention?
Can I use always camel case?
@azlam-abdulsalam
@dieffrei I'd be very careful here as I don't believe the flag is useful in the scenario where an actual error is thrown by the CLI. If you don't provide the flag and ignore that error, you will FAIL installing the packages to PROD. |
@dieffrei This needs a proper DR before proceeding |
If package was already promoted, fail instead of ignore
Checklist
All items have to be completed before a PR is merged