Uh oh!
There was an error while loading. Please reload this page.
build: initial release staging script - #13621
Conversation
668a029 to
fe35844Compare| * on the working directory of the current process is not good because it's not guaranteed | ||
| * that the working directory is always the target project directory (e.g. w/ bazel run). | ||
| */ | ||
| export class GitCommandExecutor { |
There was a problem hiding this comment.
Maybe GitClient? "CommandExecutor" makes me think you would pass it full commands to run rather than using API methods directly.
"Executor" also makes me think of something else...
There was a problem hiding this comment.
Ugh haha 😅- GitClient sounds better yeah.
| /** Returns the currently checked out branch for the current working directory. */ | ||
| getCurrentBranch() { | ||
| return spawnSync('git', ['symbolic-ref', '--short', 'HEAD'], {cwd: this.projectDir}) |
There was a problem hiding this comment.
git name-revmight be better here (?). Or maybe just shorter ¯\(ツ)/¯
There was a problem hiding this comment.
It seems that git name-rev is not returning a short variant of the symbol name.
This is the output: HEAD build/release-automation-script. With symbolic-ref we just get the actual branch name and can read from stdout.
| constructor(public projectDir: string) {} | ||
| /** Returns the currently checked out branch for the current working directory. */ |
There was a problem hiding this comment.
nit: prefer "Gets ..." to "Returns ..."
| if (!currentVersion) { | ||
| console.warn(red(`Cannot parse current project version. This means that we cannot validate ` + | ||
| `the new ${bold('custom')} version that will be specified.`)); |
There was a problem hiding this comment.
Should we just throw in this case? Why would we ever want to continue if this fails?
| public minor: number, | ||
| public patch: number, | ||
| public suffix?: string, | ||
| public suffixNumber?: number) {} |
There was a problem hiding this comment.
How aboutsuffix -> prereleaseLabelsuffixNumber -> prereleaseNumber
A comment that breaks down the different parts would also be good to include here.
| newVersion.major === previousVersion.major && | ||
| newVersion.minor === previousVersion.minor && | ||
| newVersion.patch === previousVersion.patch; | ||
| } |
There was a problem hiding this comment.
These last four cases could be written a bit more concisely like:
exportfunctionvalidateExpectedVersion(newVersion: VersionInfo,previous: VersionInfo,releaseType: ReleaseType): boolean{letexpectedVersion;if(releaseType==='major'){expectedVersion=newVersionInfo(previous.major+1,0,0);}elseif(releaseType==='minor'){expectedVersion=newVersionInfo(previous.major,previous.minor+1,0);}elseif(releaseType==='patch'){expectedVersion=newVersionInfo(previous.major,previous.minor,previous.patch+1);}elseif(releaseType==='stable'){expectedVersion=newVersionInfo(previous.major,previous.minor,previous.patch);}returnnewVersion.equals(expectedVersion);}(with the corresponding addition of the equals method on VersionInfo)
There was a problem hiding this comment.
This check is gone completely now.
| /** Regular expression that matches version names and the individual version segments. */ | ||
| const versionNameRegex = /^(\d+)\.(\d+)\.(\d+)(?:-(alpha|beta|rc)\.(\d)+)?$/; | ||
| export class VersionInfo { |
| @@ -0,0 +1,27 @@ | |||
| import {VersionInfo} from './parse-version'; | |||
| export type VersionType = 'major' | 'minor' | 'patch'; | |||
There was a problem hiding this comment.
ReleaseType? A version on its own doesn't inherently have a "type" without the context of the versions around it.
There was a problem hiding this comment.
We already have ReleaseType. A release type can be either: Major Release, Minor Release, Patch Release, Pre-Release, Stable release or Custom release.
Also, isn't each version that follows Semver either MajorMinor or Patch? This is similar to how we have it within the Gulp setup.
I think this is also made clear by the actual Semver rules: e.g. here
| * Creates a new version from the specified version based on the given target version type. | ||
| * If no target version type has been specified, just the version suffix will be removed. | ||
| */ | ||
| export function createNewVersion(currentVersion: VersionInfo, targetVersionType?: VersionType): |
There was a problem hiding this comment.
Is there ever a time when the result of this function would be different from expected version for validateExpectedVersion it seems like it could be the same code.
| } | ||
| return new VersionInfo( | ||
| parseInt(matches[1]), |
There was a problem hiding this comment.
Prefer Number to parseInt here
a1007cc to
4b8738fCompareUh oh!
There was an error while loading. Please reload this page.
| "@types/gulp": "3.8.32", | ||
| "@types/gulp-util": "^3.0.34", | ||
| "@types/hammerjs": "^2.0.35", | ||
| "@types/inquirer": "^0.0.43", |
There was a problem hiding this comment.
Add an npm scripts alias for stage release that provides the project directory for you?
(might need a small wrapper script)
There was a problem hiding this comment.
Done. We needed a wrapper script because npm scripts do not interpolate environment variables. This should be fine for now.
I will definitely look for ways to improve this, but should be good for an initial implementation.
Uh oh!
There was an error while loading. Please reload this page.
| console.log(cyan('-----------------------------------------')); | ||
| console.log(); | ||
| const newVersion = await promptForNewVersion(this.currentVersion); |
There was a problem hiding this comment.
Is there anything that validates this new version? Mainly for the case where a custom version is entered.
There was a problem hiding this comment.
Yeah, the custom version will be already validated by the validate function which you've simplified with the comment above.
There was a problem hiding this comment.
I meant more that the version selected is the correct version, not that it's a valid version. E.g., entering 1.0.0 being incorrect.
This could go in a follow-up, though.
There was a problem hiding this comment.
Ah I see, so a custom version can be any valid Semver version that is more recent than the current version right?
Otherwise, if we add more limitations, we could probably remove the "custom-release" once there is a way to create pre-releases with a label
There was a problem hiding this comment.
Yeah, removing it might be better because I can't think of a scenario where you'd want to do something abnormal.
There was a problem hiding this comment.
I agree. I will remove it in the follow-up that will allow creating a pre-release with custom label.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
* Creates an initial release staging script with prompts.
03fa96e to
48a4edcComparegooglebot
commented
Oct 16, 2018
CLAs look good, thanks! |
| console.log(cyan('-----------------------------------------')); | ||
| console.log(); | ||
| const newVersion = await promptForNewVersion(this.currentVersion); |
There was a problem hiding this comment.
I meant more that the version selected is the correct version, not that it's a valid version. E.g., entering 1.0.0 being incorrect.
This could go in a follow-up, though.
Implements the functionality to select a pre-release label (as discussed in: angular#13621 (comment))
Implements the functionality to select a pre-release label (as discussed in: angular#13621 (comment))
Implements the functionality to select a pre-release label (as discussed in: angular#13621 (comment))
Implements the functionality to select a pre-release label (as discussed in: angular#13621 (comment))
Implements the functionality to select a pre-release label (as discussed in: #13621 (comment))
Implements the functionality to select a pre-release label (as discussed in: #13621 (comment))
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
@jelbourn This is an initial version. There are still a few things like the changelog that I want to address. But those should be additional PRs. -- For now, I've kept the existing Gulp publish stuff until this is finished completely.