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

adding release comment management to all config.go #2917

Merged
merged 3 commits into from
Sep 26, 2019

Conversation

balopat
Copy link
Contributor

@balopat balopat commented Sep 20, 2019

Related to #2775.

Description

This "master PR" adds automation around managing comments on the config.go files that inform the user about whether the latest config.go is safe to modify or not (depending on whether it's released or not).

The plan is to break this up to smaller PRs. However feedback is welcome here, and it is a working version that you can test for UX.

User facing changes

Only contributor facing changes.

Before

Currently the hack/check-schema-changes.sh script is the only way to know if the config that you're modifying is released or not (or checking manually).

After

This PR adds:

  • a helper comment on all the currently released config.go files
// !!! WARNING !!! This config version is already released, please DO NOT MODIFY the structs in this file.
  • a helper comment on the current latest (v1beta15) that it is unreleased:
// This config version is not yet released, it is SAFE TO MODIFY the structs in this file.
  • a script that updates these comments in bulk: go run hack/versions/cmd/update_comments/main.go
  • a script that marks the latest version released - and it is called from release.sh so that we flip the latest version to released
  • update to the new_config script to make sure the new config release has the unreleased comment
  • fixed go_diff.go to add interface{} type support that is in v1beta5

Next PRs.

to break this up to smaller PRs:

  • (2 files) fix go_diff.go to add interface{} type support that is in v1beta5
  • (6 files) the new comment management logic and hooking it up to release and new_config
  • (20 files) update of the comments historically on all config.go files

To finish #2775:

  • document the process in DEVELOPMENT.md

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Mentions any output changes.
  • Adds documentation as needed: user docs, YAML reference, CLI reference.
  • Adds integration tests if needed.

Reviewer Notes

  • The code flow looks good.
  • Unit test added.
  • User facing changes look good.

@codecov
Copy link

codecov bot commented Sep 20, 2019

Codecov Report

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

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

I am going to Large PR Police and ask you to break it into 3 PRs :)

  1. support interface type in AST
  2. Add the new script to add commit
  3. Finally all the changes.

Thank you!

shouldErr: false,
},
{
name: "supported types",
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you added support for interface Type? is this test case for that that?

Suggested change
name: "supported types",
name: "supported interface types",

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

LGTM.

hack/versions/pkg/schema/comment.go Outdated Show resolved Hide resolved
hack/versions/pkg/schema/comment_test.go Show resolved Hide resolved
Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

LGTM

hack/versions/pkg/schema/comment.go Outdated Show resolved Hide resolved
@balopat balopat merged commit 1c372c0 into GoogleContainerTools:master Sep 26, 2019
@tejal29 tejal29 mentioned this pull request Oct 4, 2019
4 tasks
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.

4 participants