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

Improve versioning #2798

Merged
merged 4 commits into from
Sep 6, 2019
Merged

Conversation

balopat
Copy link
Contributor

@balopat balopat commented Sep 4, 2019

This is regarding #2775.

  • hack/new_version now checks via the Github API whether the current version is released or not - if not then fails
  • hack/new_version.sh now takes the new version as an argument for easier testing, e.g, try it out with hack/new_version.sh v1beta15!

next PRs will cater for :

  • add a PR check that uses this release logic to ensure that no released version is mutated

@balopat balopat added the priority/p0 Highest priority. We are actively looking at delivering it. label Sep 4, 2019
@codecov
Copy link

codecov bot commented Sep 4, 2019

Codecov Report

Merging #2798 into master will not change coverage.
The diff coverage is n/a.

@balopat balopat added the kokoro:run runs the kokoro jobs on a PR label Sep 4, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Sep 4, 2019
Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

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

Could you create a separate PR to increase the test timeout?

Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

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

I'm not sure I like the usage of an alias for latest. We've been there and it's harder to use. For example, you don't get auto import from the IDE when you write latest.SkaffoldConfig{}

@briandealwis
Copy link
Member

Could the release.sh script compare .../latest/config.go — or the generated schema? — since the last tagged release?

The last tagged release is available from git describe --match 'v[0-9]*' --abbrev 0 (source).

@balopat
Copy link
Contributor Author

balopat commented Sep 4, 2019

I'm not sure I like the usage of an alias for latest. We've been there and it's harder to use. For example, you don't get auto import from the IDE when you write latest.SkaffoldConfig{}

The autoimport is a good point. I think the latest pointer is not a vital thing, I can introduce the Released flag without it. I'll split out the timeout increase!

@balopat
Copy link
Contributor Author

balopat commented Sep 4, 2019

Could the release.sh script compare .../latest/config.go — or the generated schema? — since the last tagged release?

The last tagged release is available from git describe --match 'v[0-9]*' --abbrev 0 (source).

That could work too, thanks for the suggestion!
Pros: it can't be spoofed + no need for an extra bool flag.
Cons: now the contributor won't have a visual confirmation before trying to add a new field - but we can still add a comment before the config version though in the release script.

@balopat balopat added the kokoro:run runs the kokoro jobs on a PR label Sep 4, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Sep 4, 2019
@balopat balopat requested a review from dgageot September 5, 2019 05:20
Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

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

LGTM with a few nits

hack/new_version.sh Show resolved Hide resolved
hack/new_version.sh Outdated Show resolved Hide resolved
hack/new_config_version/version.go Outdated Show resolved Hide resolved
hack/new_config_version/version.go Outdated Show resolved Hide resolved
@balopat balopat merged commit eae79c8 into GoogleContainerTools:master Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes priority/p0 Highest priority. We are actively looking at delivering it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants