Skip to content

Conversation

rose-m
Copy link
Contributor

@rose-m rose-m commented Feb 18, 2021

No description provided.

@rose-m rose-m self-assigned this Feb 18, 2021
Comment on lines 5 to 19
return new Promise((resolve, reject) => {
try {
const rl = readline.createInterface({
input: process.stdin,
output: process.stdout
});

rl.question(`${prompt} `, (answer) => {
rl.close();
resolve(answer);
});
} catch (e) {
reject(e);
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return new Promise((resolve, reject) => {
try {
const rl = readline.createInterface({
input: process.stdin,
output: process.stdout
});
rl.question(`${prompt} `, (answer) => {
rl.close();
resolve(answer);
});
} catch (e) {
reject(e);
}
});
return new Promise(resolve => {
const rl = readline.createInterface({
input: process.stdin,
output: process.stdout
});
rl.question(`${prompt} `, (answer) => {
rl.close();
resolve(answer);
});
});

(Exceptions from inside the Promise constructor’s callback are turned into rejections automatically :))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, that makes sense :D

}).filter(t => !!t) as TagDetails[];

return { commit, tags: tagDetails };
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we maybe better start out with git tag, looking for the tag that we actually care about, and then use git log -n1 --pretty='%H' <tag> to get the commit hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sounds good to me, too. Probably the main difference is that git tag gives us all tags in the repo we already know whereas the git log check returns only those from the current branch.

How is that in the case of future support releases? 🤔 <- That question I'd rather answer in a different ticket.

So changing to git tag and grabbing hash afterwards for now!

}
if (!repositoryStatus.branch.tracking) {
throw new Error('The branch you are on is not tracking any remote branch. Ensure you are on the master branch of the repository.');
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These things are actually moving us further away from being able to do support releases, right? Could we maybe prompt the user instead to make sure they’re on the right branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it depends probably on how we want to do them. At least a tracking branch should be there. Right now I've put in the master guard just for good measure. I'd assume we create a separate branch for a support release and tag on that branch. But for that it should definitely be pushed to the remote (thus we should have a tracking branch).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn’t referring to the tracking branch check specifically, sorry :) Maybe we could do some very crude regex check here, e.g. /^master$|^main$|^v\d+\.\d+\.\d+/ (main because of https://jira.mongodb.org/browse/MONGOSH-245)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea 👍 So we'd cover master/main and potential support release branches (which would equal to the designated version number)... ✅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah … and maybe also allow [xyz0-9] instead of \d, I don’t think we have any idea what the branch naming scheme will look like but using things like v0.x.y or v0.8.x would be common

cwd: repositoryRoot,
encoding: 'utf-8'
});
spawnSync('git', ['push', '--tags'], {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would maybe recommend only pushing the tag that we created here, instead of any tags the user might have lying around as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I currently fail if the user has unpushed tags - so here it would not push anything additional. But good point, might change that to only push that specific tag.

@rose-m rose-m requested a review from addaleax February 18, 2021 16:13
@rose-m rose-m merged commit f2734d7 into master Feb 18, 2021
@rose-m rose-m deleted the MONGOSH-530-local-release-tasks branch February 18, 2021 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants