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

API release automation with go script #8484

Merged
merged 14 commits into from
Oct 28, 2021

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Oct 6, 2021

This PR is the successor to #7598. It is functionally the same to the previous version except that it now uses a go program instead of bash scripting. The initial bash script version was veto'd because a go program will be more readable, maintainable, and less bug prone, especially since it can use golang.org/x/mod/modfile, golang.org/x/tools/go/ast/astutil, and golang.org/x/tools/go/packages.

Note: The go program is not currently to go backwards from v2+ to v0/v1.

Description:

As a part of the API RFD...

This PR adds the update-api-module-path make target to update the api module path to the current major version.e.g. gravitational/teleport/api -> gravitational/teleport/api/v7.

The program only makes changes if:
1. the major version of the API changes
2. no version suffixes are present dev|alpha|beta|rc
- This way, the import path is only updated on an actual release, which avoids backport and merge-conflict issues during the pre-release rush.

update-api-module-path runs at the end of make version, so the api version should be automatically updated with the current release processes in place.

Other changes:

  • since protoc does not understand go modules, and reads the version suffix as a directory, protoc now reads from vendor/.../teleport/api/vX.
    • This make it so .proto can be updated with the new api module path, and pb.go will be properly generated.

Example of changes made when you update the version and run make version - #8673

@Joerger Joerger force-pushed the joerger/joerger/api-release-with-go branch from ef976aa to 7a89d66 Compare October 6, 2021 00:46
@Joerger Joerger mentioned this pull request Oct 6, 2021
Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

I missed the discussion on the RFD, but I'm a little concerned that we're going against the general advice:

A module developer should increment this number past v1 only when necessary because the version upgrade represents significant disruption for developers whose code uses function in the upgraded module. This disruption includes backward-incompatible changes to the public API, as well as the need for developers using the module to update the package path wherever they import packages from the module.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
build.assets/Dockerfile Outdated Show resolved Hide resolved
build.assets/update-api-module-path/main.go Outdated Show resolved Hide resolved
build.assets/update-api-module-path/main.go Outdated Show resolved Hide resolved
build.assets/update-api-module-path/main.go Outdated Show resolved Hide resolved
build.assets/update-api-module-path/main.go Outdated Show resolved Hide resolved
build.assets/update-api-module-path/main.go Outdated Show resolved Hide resolved
@Joerger
Copy link
Contributor Author

Joerger commented Oct 6, 2021

I missed the discussion on the RFD, but I'm a little concerned that we're going against the general advice:

A module developer should increment this number past v1 only when necessary because the version upgrade represents significant disruption for developers whose code uses function in the upgraded module. This disruption includes backward-incompatible changes to the public API, as well as the need for developers using the module to update the package path wherever they import packages from the module.

I'm with you. I definitely wanted to avoid it, but it makes things a lot simpler for customers and developers to have Teleport client and server version aligned. Our version compatibility rules are already hard for customers and developers to follow at times, so introducing a completely new versioning guideline for the API will increase the room for errors/issues to occur. Ultimately the pros and cons were weighed and this was the consensus.

It's definitely not too late to reconvene and see if we can make another approach. I still think that Release Option 2 from the RFD would give a good in between.

However, the API module has been complete but has yet to be released so I'm concerned about pushing it back longer. Teleport plugins has to import specific commits which is hard to follow and maintain. This is an even bigger problem for customers who want to import a specific version of the API and have no feasible way to do so without our help.

@Joerger Joerger requested a review from zmb3 October 6, 2021 22:36
@Joerger Joerger requested review from atburke and removed request for andrejtokarcik October 20, 2021 19:07
@Joerger
Copy link
Contributor Author

Joerger commented Oct 20, 2021

@zmb3 @atburke @webvictim @tcsc If any of you have time this week, could you review this?

These changes were largely reviewed and approved in #7598 and in the API RFD.

I would be very happy if we could get this into 8.0 or an early Minor release since importing the API is not easy for users without proper versioning.

@Joerger Joerger force-pushed the joerger/joerger/api-release-with-go branch from fd17eec to 049fb90 Compare October 20, 2021 19:28
@Joerger Joerger mentioned this pull request Oct 20, 2021
Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

Sorry it took so long to get back to this.

Some final thoughts - it seems if this tool fails at any point, it doesn't attempt to undo any of the actions it took and leave the system in the last known good state.

If we expect developers to run this tooling on their dev machines, and the tool makes changes that cant be quickly reverted with git, then maybe it makes sense to expand the error handling a bit.

Lastly, some tests that exercise this functionality would help make it easier to understand what this tool does and what things should look like before and after a run.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
build.assets/update-api-module-path/main.go Outdated Show resolved Hide resolved
build.assets/update-api-module-path/main.go Outdated Show resolved Hide resolved
build.assets/update-api-module-path/main.go Outdated Show resolved Hide resolved
build.assets/update-api-module-path/main.go Outdated Show resolved Hide resolved
build.assets/update-api-module-path/main.go Outdated Show resolved Hide resolved
@Joerger Joerger force-pushed the joerger/joerger/api-release-with-go branch 3 times, most recently from 30d5c26 to 55c3a47 Compare October 21, 2021 21:19
@Joerger Joerger requested a review from zmb3 October 21, 2021 21:19
Makefile Show resolved Hide resolved
mkdir -p $(shell dirname $(API_VENDOR_PATH))
# make a relative link to the true api dir (without using `ln -r` for non-linux OS compatibility)
if [ -d $(shell dirname $(API_VENDOR_PATH))/../../../../../api/ ]; then \
ln -s ../../../../../api $(API_VENDOR_PATH); \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to write a platform-independent version in Go, perhaps?

build.assets/update_api_module_path/main_test.go Outdated Show resolved Hide resolved
@Joerger Joerger force-pushed the joerger/joerger/api-release-with-go branch from c7e6323 to 35b2cc9 Compare October 25, 2021 17:40
Copy link
Contributor

@russjones russjones left a comment

Choose a reason for hiding this comment

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

Bot.

@Joerger Joerger enabled auto-merge (squash) October 26, 2021 22:25
@Joerger Joerger merged commit 20da22c into master Oct 28, 2021
@Joerger Joerger deleted the joerger/joerger/api-release-with-go branch October 28, 2021 17:15
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.

6 participants