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

Fail PR if it has a structural schema change in a released version #2864

Merged
merged 9 commits into from Sep 13, 2019

Conversation

@balopat
Copy link
Collaborator

balopat commented Sep 12, 2019

The most important part of #2775.

I broke it up to 2 PRs - this one is going to be the second one, should be rebased and merged AFTER the refactoring one: #2867

It checks whether the PR compared to master contains any changes in the config.go files under pkg/skaffold/schema. If yes, it checks if those changes are structural changes or not. If they are and the package is not "latest", then we'll fail the PR as we assume anything else than latest is already released and shouldn't be changed. If the change is latest and it is released, we fail the PR for the same reason. If the change is in latest and it is not released yet, it is fine to make changes.

I decided to compare against master. This enables to override this check in case it gets in the way (some kind of historical refactoring that this check recognizes as a structural change incorrectly, or in case we do want to introduce a structural change historically) - if a maintainer has Admin rights they can merge it still.

detected structural change in released version:

Example Travis Build on this PR: https://travis-ci.com/GoogleContainerTools/skaffold/jobs/234305164

image

detected structural changes in unreleased latest:

Example Travis Build on this PR: https://travis-ci.com/GoogleContainerTools/skaffold/jobs/234305550

image

structural change in latest that is also released (i.e. on a PR that tries to modify config after a release):

Example Travis Build on this PR (simulated by temporarily switching the released logic):
https://travis-ci.com/GoogleContainerTools/skaffold/jobs/234306518

image

Error handling when failed to list Github repos:

image

@googlebot googlebot added the cla: yes label Sep 12, 2019
@balopat balopat changed the title Fail PRs if it has a structural schema change in a released version Fail PR if it has a structural schema change in a released version Sep 12, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 12, 2019

Codecov Report

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

@balopat balopat force-pushed the balopat:check_ast_change branch 2 times, most recently from 584a881 to 8d2736e Sep 12, 2019
hack/check-schema-changes.sh checks whether the PR compared to master contains any changes in the config.go files under pkg/skaffold/schema. If yes, it checks if those changes are structural changes or not. If they are and the package is not "latest", then we'll fail the PR as we assume anything else than latest is already released and shouldn't be changed. If the change is latest and it is released, we fail the PR for the same reason. If the change is in latest and it is not released yet, it is fine to make changes.
@balopat balopat force-pushed the balopat:check_ast_change branch from 1501dd2 to a7a5970 Sep 12, 2019
hack/check-schema-changes.sh Outdated Show resolved Hide resolved
hack/check-schema-changes.sh Outdated Show resolved Hide resolved
hack/check-schema-changes.sh Outdated Show resolved Hide resolved
hack/versions/cmd/new/new.go Outdated Show resolved Hide resolved
hack/versions/pkg/diff/godiff.go Show resolved Hide resolved
hack/versions/pkg/diff/godiff.go Show resolved Hide resolved
hack/versions/pkg/diff/godiff.go Show resolved Hide resolved
hack/versions/pkg/version.go Show resolved Hide resolved
@balopat balopat removed the needs-rebase label Sep 12, 2019
@balopat balopat force-pushed the balopat:check_ast_change branch from b6e604b to 6a6418d Sep 12, 2019
balopat added 2 commits Sep 12, 2019
Copy link
Member

tejal29 left a comment

Let me know what you think for changing this script to just check for latest/config.go.
Lets try to get this in today, we can test it against an incoming PR #2871

hack/versions/pkg/schema/git.go Show resolved Hide resolved
hack/versions/pkg/schema/check.go Show resolved Hide resolved
hack/versions/pkg/schema/check.go Show resolved Hide resolved
@balopat balopat merged commit c24d84d into GoogleContainerTools:master Sep 13, 2019
3 checks passed
3 checks passed
Travis CI - Pull Request Build Passed
Details
cla/google All necessary CLAs are signed
kokoro CI build successful.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.