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

Repo/test manifest divergence #797

Merged
merged 3 commits into from
Aug 11, 2020
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Aug 7, 2020

What this PR does / why we need it:

Adds CI task to run the manifest generation script, check if it resulted in changes, and fail if it did. This ensures that single and base manifests do not diverge.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #668

Special notes for your reviewer:
https://github.com/Kong/kubernetes-ingress-controller/runs/960118920?check_suite_focus=true failed because it should. The manifests aren't in sync currently. In a local test directory where I have updated them:

15:20:02-0700 yagody $ git status
On branch repo/test-manifest-divergence
nothing to commit, working tree clean
15:20:04-0700 yagody $ ./hack/build-single-manifests.sh 
15:20:21-0700 yagody $ git status                      
On branch repo/test-manifest-divergence
nothing to commit, working tree clean
15:20:22-0700 yagody $ make verify-manifests
./hack/verify-manifests.sh
diffing ./hack/../deploy/single/ against freshly generated single manifests
./hack/../deploy/single/ up to date.

How do we want to handle that? Just update them and roll the changes into this PR, or separate PR to update them and then rebase this onto next (or main) after it's merged?

@rainest rainest requested a review from hbagdi as a code owner August 7, 2020 22:23
@hbagdi hbagdi changed the base branch from main to next August 7, 2020 23:40
@hbagdi hbagdi requested a review from mflendrich August 7, 2020 23:41
hack/verify-manifests.sh Outdated Show resolved Hide resolved
hack/verify-manifests.sh Outdated Show resolved Hide resolved
hack/verify-manifests.sh Outdated Show resolved Hide resolved
hack/verify-manifests.sh Outdated Show resolved Hide resolved
uses: imranismail/setup-kustomize@v1
with:
kustomize-version: "3.1.0"
- name: Verify manifest consistency
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this check is failing at introduction 😁

Copy link
Contributor

@mflendrich mflendrich Aug 10, 2020

Choose a reason for hiding this comment

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

Ah, should have read Special notes for your reviewer first. 🤦

How do we want to handle that?

I recommend a TDD-like approach here:

  1. introduce a failing test
  2. fix the cause of failure
  3. anticipate test success 🎉

For me, this can (or even should) happen in 1 PR. It does not feel right to deliberately introduce a CI failure. I won't insist, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, mostly a question of just where that change goes, which is a minor procedural point and not particularly important.

Given that it's pretty minor and I have the link to an existing test run demonstrating the failure already, going ahead and pushing the manifest updates within this PR.

Travis Raines and others added 2 commits August 10, 2020 10:58
@rainest
Copy link
Contributor Author

rainest commented Aug 10, 2020

Latest updates apply @mflendrich 's bash suggestions and comingle a manifest update along with the CI update so that the test now passes. Please refer to the run linked earlier/CI history for a demonstration of the failure case behavior.

@mflendrich mflendrich merged commit f901e55 into next Aug 11, 2020
@mflendrich mflendrich deleted the repo/test-manifest-divergence branch August 11, 2020 11:38
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.

Catch errors in divergence of single and base manifests
2 participants