Skip to content
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 version numbers for v1.0.0-rc1 #247

Merged
merged 2 commits into from
Feb 5, 2020

Conversation

rartino
Copy link
Contributor

@rartino rartino commented Feb 2, 2020

This PR is meant to be the final PR before we merge into master to release v1.0.0-rc1. (Note: #246 needs to be merged before this one.)

It updates the main version number to v1.0.0-rc1, and makes related changes to our examples. (It would seem odd to keep all examples using versioned URLs under v0.9 when the present specification is at v1.)

Note how I file this PR against develop rather than master. The reason is that for v1.0.0-rc1 and forward I suggest we slightly alter our handling of version numbers in the release procedure compared to what is documented in our wiki to be as follows:

  1. We first merge all other PRs against develop that should go into the release.
  2. We merge the present PR that updates the main version number.
  3. We use GitHub's UI to merge develop into master. The branches should be set up so this is a straightforward fast-forwardable merge. HEAD of master will thus move directly from v0.10.1 to v1.0.0-rc1.
  4. We use the GitHub release feature to tag HEAD of master as v1.0.0-rc1.
  5. We file a PR against develop to update the version number to v1.0.1-develop.
  6. From now onwards we can start merging new PRs in develop.

The reason I propose this workflow is that then the state of master will always be a proper named release, even if the release-related PRs sit a day or two waiting to be reviewed and approved. Hence, there is no need for "release meetings", and people can take the time they want to check the release PRs for errors.

If someone sees a reason not to adopt this procedure, please voice your concerns.

@CasperWA
Copy link
Member

CasperWA commented Feb 2, 2020

Note how I file this PR against develop rather than master. The reason is that for v1.0.0-rc1 and forward I suggest we slightly alter our handling of version numbers in the release procedure compared to what is documented in our wiki to be as follows:

Sounds good! Slight technical comments below that I've found concerning GitHub.

  1. We first merge all other PRs against develop that should go into the release.
  2. We merge the present PR that updates the main version number.
  3. We use GitHub's UI to merge develop into master. The branches should be set up so this is a straightforward fast-forwardable merge. HEAD of master will thus move directly from v0.10.1 to v1.0.0-rc1.

I haven't found a way to do this via GitHub's UI, so I suggest to make the PR on GitHub and then a single person does git merge --ff-only locally and push to master - this will also render the GitHub PR "merged".

  1. We use the GitHub release feature to tag HEAD of master as v1.0.0-rc1.

Again, this can be done via git, but I support using GitHub for this (as is also suggested), since the release comments can be done more elegantly than via a tag message - just wanted to get a reason in here for using GitHub instead of adding a tag locally and pushing it.

  1. We file a PR against develop to update the version number to v1.0.1-develop.

Shouldn't be needed if the prior merge is done via git merge --ff-only as suggested above, since the commit UUIDs will be the same (which they wouldn't be if rebasing) and there will be no merge commit (since it will be fast-forward). I don't know about the release tag though, but it shouldn't matter I think? Otherwise one can create the PR and merge it with git merge --ff-only as above.

  1. From now onwards we can start merging new PRs in develop.

The reason I propose this workflow is that then the state of master will always be a proper named release, even if the release-related PRs sit a day or two waiting to be reviewed and approved. Hence, there is no need for "release meetings", and people can take the time they want to check the release PRs for errors.

If someone sees a reason not to adopt this procedure, please voice your concerns.

No concerns - seems sound 👍

@CasperWA
Copy link
Member

CasperWA commented Feb 2, 2020

Also, there were a lot of v0.9's in the spec that shouldn't have been there 😮

@rartino
Copy link
Contributor Author

rartino commented Feb 3, 2020

(Edit: in my first read I missed your statements about how the command line would interact with the GitHub PR, so I edited my response accordingly.)

I haven't found a way to do this via GitHub's UI, so I suggest to make the PR on GitHub and then a single person does git merge --ff-only locally and push to master - this will also render the GitHub PR "merged".

Oh, that is annoying. I really would like to avoid a merge commit if possible so we don't have to merge master back into develop. But if the interaction with merging GitHub PR works automatically as you seem to suggest, I suppose it is still ok? We should just document the steps.

 5. We file a PR against develop to update the version number to v1.0.1-develop.

Shouldn't be needed if the prior merge is done via git merge --ff-only as suggested above, since the commit UUIDs will be the same (which they wouldn't be if rebasing) and there will be no merge commit (since it will be fast-forward). I don't know about the release tag though, but it shouldn't matter I think? Otherwise one can create the PR and merge it with git merge --ff-only as above.

Did you perhaps misread this? The PR in step 5 isn't to merge master back into develop, it is a first commit to re-instate the -develop flag on the version number inside optimade.rst.

@merkys
Copy link
Member

merkys commented Feb 3, 2020

The procedure of handling version numbers sounds good to me. I trust more experienced GitHub users to review the details of the merging procedure.

@CasperWA
Copy link
Member

CasperWA commented Feb 3, 2020

(Edit: in my first read I missed your statements about how the command line would interact with the GitHub PR, so I edited my response accordingly.)

I haven't found a way to do this via GitHub's UI, so I suggest to make the PR on GitHub and then a single person does git merge --ff-only locally and push to master - this will also render the GitHub PR "merged".

Oh, that is annoying. I really would like to avoid a merge commit if possible so we don't have to merge master back into develop. But if the interaction with merging GitHub PR works automatically as you seem to suggest, I suppose it is still ok? We should just document the steps.

I will try and update the wiki accordingly 👍

  1. We file a PR against develop to update the version number to v1.0.1-develop.

Shouldn't be needed if the prior merge is done via git merge --ff-only as suggested above, since the commit UUIDs will be the same (which they wouldn't be if rebasing) and there will be no merge commit (since it will be fast-forward). I don't know about the release tag though, but it shouldn't matter I think? Otherwise one can create the PR and merge it with git merge --ff-only as above.

Did you perhaps misread this? The PR in step 5 isn't to merge master back into develop, it is a first commit to re-instate the -develop flag on the version number inside optimade.rst.

I did indeed misread this - disregard my previous comment.
On another note, if we are updating all this in any case, do we still want to maintain the develop suffix? An increase in the patch version should be enough, right? Even if the version that is pushed to master will be something different, this should still be enough of a difference.

@CasperWA
Copy link
Member

CasperWA commented Feb 3, 2020

Updated the wiki: link

@rartino
Copy link
Contributor Author

rartino commented Feb 3, 2020

So, I see no protests. Let's get the release out.

@rartino rartino added the PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing label Feb 3, 2020
@merkys
Copy link
Member

merkys commented Feb 3, 2020

What about the version numbers in OpenAPI specification JSON files? They still refer to 0.10.1.

@CasperWA
Copy link
Member

CasperWA commented Feb 3, 2020

What about the version numbers in OpenAPI specification JSON files? They still refer to 0.10.1.

True - I'll create a quick updating PR to be merged prior to this one.

Edit: I have found that there are several (we create /optimade, /optimade/v0.10, and /optimade/v0.10.1 base URLs - should be updated according to #240) updates that needs to be done - I will update optimade-python-tools and generate a new spec.

Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Let's set a Skype time for quickly releasing this, right? Even if it's just one person pushing the buttons, it's nice to have multiple sets of eyes and ears on the action.

@rartino
Copy link
Contributor Author

rartino commented Feb 3, 2020

Let's set a Skype time for quickly releasing this, right?

Isn't the point that if we follow the new procedure, it isn't really needed?

After this PR, there will be a PR to merge develop into master. That's when the multiple sets of eyes have their last chance to stop the release, so we may want to let that sit for a day. In the comments on that PR (while waiting for two approvals) we agree on who does the git FF merge, and then it is just the matter of executing that one command and the release is out.

Going forward I just think it is convenient to agree on, and test, a workflow where we can release safely without having to coordinate a Skype meeting. It would certainly help bugfix patch releases.

@rartino
Copy link
Contributor Author

rartino commented Feb 3, 2020

If we send out an email about the final merge PR with a link to the specification that is about to be released and say that it will happen in 24h, we are even likely to get more eyes on it than we would on a Skype meeting. There is a significant risk that will lead to a few additional last-minute fixes though, but I suppose that is a good thing.

@CasperWA
Copy link
Member

CasperWA commented Feb 3, 2020

Let's set a Skype time for quickly releasing this, right?

Isn't the point that if we follow the new procedure, it isn't really needed?

After this PR, there will be a PR to merge develop into master. That's when the multiple sets of eyes have their last chance to stop the release, so we may want to let that sit for a day. In the comments on that PR (while waiting for two approvals) we agree on who does the git FF merge, and then it is just the matter of executing that one command and the release is out.

Going forward I just think it is convenient to agree on, and test, a workflow where we can release safely without having to coordinate a Skype meeting. It would certainly help bugfix patch releases.

Sounds good. It was also mainly due to the release being a bump of the major version, as well as the first time with this particular workflow. But yeah - if we can do it in the next PR, no problem.

@CasperWA
Copy link
Member

CasperWA commented Feb 3, 2020

If we send out an email about the final merge PR with a link to the specification that is about to be released and say that it will happen in 24h, we are even likely to get more eyes on it than we would on a Skype meeting. There is a significant risk that will lead to a few additional last-minute fixes though, but I suppose that is a good thing.

Yeah. Let's do that. What 24h should we set aside then? The next ones? Maybe until tomorrow at 18:00 CET or something?

@rartino
Copy link
Contributor Author

rartino commented Feb 3, 2020

I was thinking 24h from the PR being posted for merge develop into master.

Or should we be super concerned that we cannot accept the present version bump PR until people have had a 24h window?

Admittedly there is a risk we get a few fixes into develop after bumping the version number.

So maybe it is better to send out that mail for accepting this PR, and then the merge PR is meant to be accepted without delay?

@CasperWA
Copy link
Member

CasperWA commented Feb 3, 2020

So maybe it is better to send out that mail for accepting this PR, and then there merge PR is meant to be accepted without delay?

That was kind of what I assumed with this workflow. All of the steps mentioned (maybe except the first one) are done in the same moment sequentially, i.e., creating the merge PR will be done shortly before merging it.
This PR, where the version is bumped, is the "critical" PR in a way, since the merging of this PR will set in motion the next steps immediately.
But maybe I projected something unspecified on the proposed workflow?

@rartino
Copy link
Contributor Author

rartino commented Feb 3, 2020

I agree with you, we should agree on that the version bumping PR is the critical one. But, there isn't any particular need for the PR that merges develop into master to go fast. We just decide to not accept any other PRs between this PR being accepted and that one. If anything critical comes up in that window, we fix it quickly but bump the version number another step.

@rartino
Copy link
Contributor Author

rartino commented Feb 3, 2020

I tried to send out an email, but there seems to be some issue with the optimade email lists server at the moment (I reported the issue.) I guess we'll count 24h from when it goes out.

Copy link
Contributor

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

Thanks a lot, looks good to me, and happy to see this happening!

Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

A minor change to add a version to a previously unversioned URL, which is not necessarily blocking for rc1.

There are several other cases of (now invalid) unversioned OPTiMaDe URLs in the spec, e.g. in the description of the links endpoint: https://github.com/Materials-Consortia/OPTiMaDe/pull/247/files#diff-78b09d0af08b624d7caa4b86cbe57959L1097

Was it concretely decided to abandon the unversioned URL (which was previously mandated to provide the latest supported version, afaik)? (The relevant discussion was in #240).

optimade.rst Outdated Show resolved Hide resolved
Co-Authored-By: Matthew Evans <7916000+ml-evs@users.noreply.github.com>
@rartino
Copy link
Contributor Author

rartino commented Feb 5, 2020

There are several other cases of (now invalid) unversioned OPTiMaDe URLs in the spec, e.g. in the description of the links endpoint: https://github.com/Materials-Consortia/OPTiMaDe/pull/247/files#diff-78b09d0af08b624d7caa4b86cbe57959L1097

@ml-evs The links in the links endpoint, i.e., the base_url of implementations, should be unversioned. The only place where unversioned URLs must be changed is when we show example of access of API functions, which is no longer available on the bare base_url.

Do you see any such still remaining? I tried to look, but don't see any.

@ml-evs
Copy link
Member

ml-evs commented Feb 5, 2020

@ml-evs The links in the links endpoint, i.e., the base_url of implementations, should be unversioned. The only place where unversioned URLs must be changed is when we show example of access of API functions, which is no longer available on the bare base_url.

Okay, I guess it's up to the client to add its own version discovery mechanism to use the links endpoint.

Do you see any such still remaining? I tried to look, but don't see any.

Nope, I think it's good now!

@ml-evs ml-evs self-requested a review February 5, 2020 18:49
@rartino
Copy link
Contributor Author

rartino commented Feb 5, 2020

Okay, I guess it's up to the client to add its own version discovery mechanism to use the links endpoint.

Well, there is a recommended implementation strategy here: https://github.com/Materials-Consortia/OPTiMaDe/blame/320ed132a393bacc844bfb4f50ff3cd509206e85/optimade.rst#L236

@rartino
Copy link
Contributor Author

rartino commented Feb 5, 2020

More than 24h since the warning email has passed. I see no release-blocking issues filed (although I am a bit worried about how major edits we'll need for #250). So, I guess we are ready to merge this! I'll press the button shortly...

@rartino rartino merged commit 94f04c0 into Materials-Consortia:develop Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants