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 the error message when a released schema is changed #4355

Merged
merged 1 commit into from
Jun 21, 2020

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Jun 19, 2020

Fixes #4168

Old message:

$ ./hack/check-schema-changes.sh
INFO[0000] structural changes in latest config, checking on Github if latest is released...
INFO[0000] Current Skaffold version: v2beta5
v2beta5 is released, it should NOT be changed!
ERRO[0000] --------
Structural change detected in a released config: pkg/skaffold/schema/latest/config.go
Please create a new PR first with a new version.
You can use 'hack/new_version.sh' to generate the new config version.
If you are running this locally, make sure you have the origin/master branch up to date!
Admin rights are required to merge this PR!
--------
diff --git a/pkg/skaffold/schema/latest/config.go b/pkg/skaffold/schema/latest/config.go
index 98f7921b4..e837615b6 100644
--- a/pkg/skaffold/schema/latest/config.go
+++ b/pkg/skaffold/schema/latest/config.go
@@ -403,6 +403,8 @@ type TestCase struct {
 type DeployConfig struct {
 	DeployType `yaml:",inline"`

+	Change bool
+
 	// StatusCheckDeadlineSeconds *beta* is the deadline for deployments to stabilize in seconds.
 	StatusCheckDeadlineSeconds int `yaml:"statusCheckDeadlineSeconds,omitempty"`

FATA[0000] structural changes detected
exit status 1

New message:

$ ./hack/check-schema-changes.sh
WARN[0000] Detected changes to the latest config. Checking on Github if it's released...
ERRO[0000] Schema "v2beta5" is already released. Changing it is not allowed.

What should I do?
-----------------
 + Please first create a new PR, with a new version, using 'hack/new_version.sh' script.
 + If you see this error and it seems incorrect, maybe you have an out-of-date "origin/master" local branch.
 + If this retroactive change is required and harmless to the users, you'll need Admin rights to merge this PR.

Invalid changes:
----------------
diff --git a/pkg/skaffold/schema/latest/config.go b/pkg/skaffold/schema/latest/config.go
index 98f7921b4..e837615b6 100644
--- a/pkg/skaffold/schema/latest/config.go
+++ b/pkg/skaffold/schema/latest/config.go
@@ -403,6 +403,8 @@ type TestCase struct {
 type DeployConfig struct {
 	DeployType `yaml:",inline"`

+	Change bool
+
 	// StatusCheckDeadlineSeconds *beta* is the deadline for deployments to stabilize in seconds.
 	StatusCheckDeadlineSeconds int `yaml:"statusCheckDeadlineSeconds,omitempty"`

exit status 1

Signed-off-by: David Gageot [email protected]

@dgageot dgageot requested a review from a team as a code owner June 19, 2020 09:33
@dgageot dgageot requested a review from nkubala June 19, 2020 09:33
@codecov
Copy link

codecov bot commented Jun 19, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4355   +/-   ##
=======================================
  Coverage   71.74%   71.74%           
=======================================
  Files         324      324           
  Lines       12506    12506           
=======================================
  Hits         8973     8973           
  Misses       2965     2965           
  Partials      568      568           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e5d7c3...bc67ba0. Read the comment docs.

hack/versions/pkg/schema/check.go Outdated Show resolved Hide resolved

"github.com/GoogleContainerTools/skaffold/hack/versions/pkg/schema"
)

func main() {
if err := schema.RunSchemaCheckOnChangedFiles(); err != nil {
logrus.Fatal(err)
os.Exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right since errors reading config files (lines 50-80) will be silently dropped.

@dgageot dgageot force-pushed the fix-4168 branch 2 times, most recently from 55fd189 to 22bd3c6 Compare June 19, 2020 16:49
@dgageot
Copy link
Contributor Author

dgageot commented Jun 19, 2020

@briandealwis Can you TAL again? Thanks!

@dgageot dgageot merged commit 0b88c49 into GoogleContainerTools:master Jun 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

check-schema-changes has confusing error when released schema is modified
4 participants