-
Notifications
You must be signed in to change notification settings - Fork 998
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
Document release steps #476
Conversation
/test test-end-to-end-batch |
docs/contributing.md
Outdated
For Feast maintainers, these are the concrete steps for making a new release. | ||
|
||
1. For a major or minor release, create and check out the release branch for the new stream, e.g. `v0.6-branch`. For a patch version, check out the stream's release branch. | ||
1. In the root `pom.xml`, remove `-SNAPSHOT` from the `<revision>` property, and commit. |
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.
Are there any other places that version strings need to be updated?
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.
Too many unfortunately. Let me see if I can source these. As long as we have the complete list we can always try to reduce it over time.
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 noticed version for Helm charts on #477
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.
Wow, it's not actually looking too bad. These are the ones I found.
- pom.xml
- infra/charts/feast/requirements.yaml
- infra/charts/feast/Chart.yaml
- infra/charts/feast/values.yaml
- infra/charts/feast/charts/feast-core/Chart.yaml
- infra/charts/feast/charts/feast-serving/Chart.yaml
docs/contributing.md
Outdated
1. Bump to the next working version and append `-SNAPSHOT` in `pom.xml`. | ||
1. Update the [CHANGELOG.md]: | ||
|
||
$ docker run -it --rm -e CHANGELOG_GITHUB_TOKEN="[Token]" -v "$(pwd)":/usr/local/src/your-app ferrarimarco/github-changelog-generator --user gojek --project feast |
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.
Perhaps we can set options to map Feast's label conventions to the generator's grouping ones, like --enhancement-labels kind/feature --bug-labels kind/bug
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.
That would be super cool.
I am just a bit worried that we havent been tagging everything diligently yet. Perhaps we should add this a bit later.
@ches I need to merge some changes into this same markdown doc, although its more structural. You will probably have to refactor your PR slightly, but it shouldnt be too hard. |
No worries, let me know when it's in. If you're not already doing something similar, as per #420 (comment) I was going to basically pull out everything above the "Development" header to a new Getting Started or Quick Start page, then tweak Contributing with some new introduction. Those are probably fairly uncontroversial changes / can be iterated on without discussion through GitBook, unlike the release procedure. |
I submitted my changes. Gitbook also has "dropdown" menu items now, so we can organize all of this information under a nice heading if you think that makes sense. Feel free to make edits as necessary. |
The release automation right now is a little bit manual still. Do you have examples of how this can be rolled into a single PR? Perhaps just making sure everyone agrees to and follows a process is a start, which is why I like this PR. Ideally this would be automated away from people, but I think we have lower hanging fruit right now.
Yes, having it be the tagged commit makes sense to me as well. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Quoting @woop from #442 because I think it's more contextual here:
I think what I'd like to suggest at this point is for someone who has actually released Feast before to follow this PR's guide to make a release for v0.3.6 / 0.3.7, fill in anything missing in my doc as you go, and send a PR against this one that fills in the gaps. Basically, I don't know what I don't know 😊 Before we postulate how process/automation could potentially be improved, let's first accurately capture how it actually works at present. Hopefully that breaks the impasse. |
Touché.
Sure, makes sense. We are also in a bit of crunch mode right now, so I do apologize for the lack of attention here. I've not gone through the complete cycle myself so I will defer to @davidheryanto and @khorshuheng here. |
It seems like the changelog generator has an For example I just ran it ChangelogUnreleasedClosed issues:
Merged pull requests:
If the above holds true, then we could use the changelog generator to create the changelog. Push it with the version string updates. Review it. Get it merged. Push the tag. But the problem is we still need the pom.xml / Snapshot addition after that, right? I don't see a way around having two PRs, unless we just want to push straight to master to add that back in. Overall I think the process you have described makes sense. Changes/additions
I can help with (2) and (3) Once we have updated the above, somebody should then walk through it. |
Generating a Change LogWe use an open source change log generator to generate change logs. The process still requires a little bit of manual effort.
Example (unmodified)
Changelog0.5.0 (2020-04-30)Breaking changes: Implemented enhancements: Fixed bugs:
Merged pull requests:
|
One open question: Which changelog do we update? Clearly we want to update the master branch, but is it sufficient to only update master and the respective release branch? If we have to update master then we would be updating the change log in three places, and we need 3 PRs. |
I've updated the guide. You might want to have a quick look. It works fine for the most part. I think we should perhaps resolve conflicts and merge this in, and then add more details on which artifacts are being produced. That might be a separate section though. |
Thanks for the updates! LGTM, I will fix the conflicts shortly. |
Separate what concerns contributors versus what concerns maintainers.
On master we still have a monolithic `docs/contributing/contributing.md` _and_ redundant copies of some of the information in broken out pages (that have missed some useful updates to the monolith). Needs to get sorted out, but going ahead and moving release process to the dedicated page that now exists for it, in part to port easily to v0.4 docs where the monolith is gone.
Rebased, and moved this to the broken out That'll get sorted out, but now it's easier to port to the v0.4-branch docs that are live on GitBook. I can send a PR for that as well. |
Thanks @ches. I can help with cleanup. For the time being I'd just like to get this merged in. We won't lose it, I promise :) |
/lgtm |
/approved |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ches, khorshuheng, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Attempts to document release procedure, and separate what concerns contributors (versioning policy and branch workflow) versus what concerns maintainers.
This doc is in GitBook but these changes need discussion: the procedure is my guess, I haven't done it so I'll need your help to correct errors and omissions.
A couple of remarks or observations with what I have here:
I'm not sure that the conventional Maven
SNAPSHOT
dance has been practiced consistently to this point. We could discuss alternatives, but so long as we intend to use it, tagged release commits ideally ought to not contain-SNAPSHOT
in the POM. I've suggested a procedure that maintains this, but it's unfortunately a lot of ceremony when you have branch protection, the snapshot dance, and changelog update (2 PRs, several redundant CI builds). Some ceremony is warranted for major releases I think, but it'd be nice if it could be one PR somehow.One redundant build is inevitable with the snapshot convention I think, even if branch protection was skipped: CI passed on latest master, but a commit is needed to remove
-SNAPSHOT
before tagging.A shortcoming with the changelog generator seems to be that, as far as I understand, the released version needs to be tagged (and pushed to GitHub?) before you run it. Ideally IMO changelog would precede the tagged commit, even makes a good candidate to be the tagged release commit along with version string updates.
Having a changelog to scan is helpful before making a version number decision, to review the Semantic Versioning implications of the release. You could run the tool in dry-run stdout mode for that I guess, but if you wanted to commit it before the act of tagging, you'll be left manually editing the file with the intended version number.
Which issue(s) this PR fixes:
Relates to #442, subsequent private conversation about whether others with certain maintainer rights are able to make releases.
Does this PR introduce a user-facing change?: