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

Fix windows build, add clarity to goreleaser build (due to race conditions). #4262

Merged
merged 2 commits into from
Nov 11, 2021

Conversation

patricknelson
Copy link
Contributor

@patricknelson patricknelson commented Oct 29, 2021

Reverts a change made by #4027 but adds clarity to goreleaser builds with more debug output in case fails occur again (such as failures which could occur from config errors that might trigger ambiguous errors which may be subject to race conditions). Also allows the ability to directly test goreleaser builds locally the same way they're currently run in Cloud Build, decoupling from cloud services (e.g. Cloud Builder, Cloud KMS and GithHub itself) by removing the actual release phase.

Please see #4028 (comment) for an in-depth explanation.

Note: To simplify review, local-build.sh is effectively just a copy of cloudbuild.sh with goreleaser release changed to goreleaser build.

@k8s-ci-robot
Copy link
Contributor

@patricknelson: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Oct 29, 2021
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 29, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @patricknelson!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 29, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @patricknelson. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 29, 2021
@patricknelson
Copy link
Contributor Author

Fixing my commits right now, will also sign the agreement in a moment.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Oct 29, 2021
@patricknelson
Copy link
Contributor Author

patricknelson commented Oct 29, 2021

I signed the agreement but it doesn't appear to be going through automatically. It should be under my personal email account, however the initial comments were using my globally configured email address.

Edit: Never mind 😄

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 29, 2021
@patricknelson
Copy link
Contributor Author

p.s. Thanks to @olljanat, since I'm not a Go developer per se, I just used a modified version of the command in #4028 (comment) for the releases documentation (using the /go/src/github.com/kubernetes-sigs/kustomize paths).

@patricknelson
Copy link
Contributor Author

/hold for a bit, need to do some research first actually before this can be merged.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 29, 2021
@k8s-ci-robot k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 29, 2021
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 29, 2021
@patricknelson
Copy link
Contributor Author

Swapping over to new Linux Foundation account (patricknelson-ebay) associated with my eBay email account instead of personal, since that will be more appropriate for this situation (dotting i's, crossing t's, etc). 🙂

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 29, 2021
@patricknelson
Copy link
Contributor Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 29, 2021
releasing/local-build.sh Outdated Show resolved Hide resolved
releasing/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

Thank you so much for digging into what is going on here and helping us improve the release process!

releasing/local-build.sh Outdated Show resolved Hide resolved
releasing/README.md Outdated Show resolved Hide resolved
…commit history) to make way for new localbuild.sh which will actually be entirely local, since this script is still very specific to Cloud Build.
@patricknelson
Copy link
Contributor Author

patricknelson commented Nov 11, 2021

Ok done @natasha41575 and @KnVerey: Retaining the original localbuild.sh filename nomenclature per #4262 (comment) and using multiple commits to ensure commit history is preserved per #4262 (comment).

Unfortunately, a side effect of those two factors above working together is that GitHub's diff viewer makes it look way more dramatic than it actually is 😕. Specifically:

  1. a143688 It shows cloudbuild-local.sh as an entirely new file, which isn't totally wrong but it's due to a rename, but the file it was renamed from still exists in the final diff.
  2. 9c4a797 It then therefore also shows a bunch of edits to localbuild.sh (since the original filename was retained) even though really it's just a carbon copy of cloudbuild.sh with just the goreleaser release command switched to goreleaser build (since that is the whole point of the local build, to unlink it from the release stage).

This is easily revealed when you look at the two commits individually, however 😄. Also, if you manually diff cloudbuild.sh with localbuild.sh you can see it's a very simple change:

$ diff cloudbuild.sh localbuild.sh
2a3,4
> # Works exactly like cloudbuild.sh but doesn't perform a release.
> #
5c7
< #  releasing/cloudbuild.sh TAG [--snapshot]
---
> #  releasing/localbuild.sh TAG [--snapshot]
14,17c16
< # Cloud build should be configured to trigger on tags
< # matching:
< #
< #   [\w/]+/v\d+\.\d+\.\d+
---
> # This script runs a build through goreleaser (http://goreleaser.com) but nothing else.
19,20d17
< # This script runs goreleaser (http://goreleaser.com),
< # presumably from a cloudbuild.yaml step that installed it.
120c117
< time /usr/local/bin/goreleaser release \
---
> time /usr/local/bin/goreleaser build \
125,126d121
<   --release-notes=$changeLogFile \
<   --rm-dist \

Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

A couple small asks/questions, then this is good to go! I'm ok with the refactor to share code between the files being a follow-up, assuming you're up for doing it. 😄
I'd like to see the local build added to our CI, but that can definitely be its own PR as well.

releasing/localbuild.sh Show resolved Hide resolved
releasing/localbuild.sh Show resolved Hide resolved
… goreleaser builds locally (localbuild.sh) in a way exactly consistent with Cloud Build (cloudbuild.sh) but as a *build* only, without being coupled to Cloud Build or it's dependencies (like Cloud KMS, GitHub, etc).
@natasha41575
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 11, 2021
@patricknelson
Copy link
Contributor Author

Apologies for the delay... ok, updates were:

  1. Fixed executable permissions on localbuild.sh and the renamed cloudbuild-local.sh
  2. Added --rm-dist back to goreleaser build (my mistake for missing that)
  3. Updated new README.md entry to just include parameters for running the build, i.e. ./releasing/localbuild.sh TAG [--snapshot]

@patricknelson
Copy link
Contributor Author

patricknelson commented Nov 11, 2021

p.s. @KnVerey I'll take a shot at those two items in the next few days when I get a chance 😄

  1. Refactor build filenames, including abstracting/simplifying config file generation to separate script, per discussion here.
  2. Add localbuild.sh to CI as well, per discussion here.

I'm much more confident about item 1, but item 2 not so much since I'm not very familiar with Makefiles, so: Quick guidance on that second item @natasha41575: I'm not finding much documentation regarding project conventions regarding mods/additions to the Makefile (might infer if I were more familiar) but I presume changes to the CI (at .github/workflows/go.yml) would also just need to be done there as well since (if I'm reading this correctly) this project is built (via that file) using make alongside lots of other k8s projects via the kubernetes/test-infra project, is that about right? 🤔 Not 100% sure I'm up to that but... heh... I might be on the right path.

@KnVerey
Copy link
Contributor

KnVerey commented Nov 11, 2021

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KnVerey, patricknelson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 11, 2021
@k8s-ci-robot k8s-ci-robot merged commit 863ff0e into kubernetes-sigs:master Nov 11, 2021
@KnVerey
Copy link
Contributor

KnVerey commented Nov 11, 2021

(if I'm reading this correctly) this project is built (via that file) using make alongside lots of other k8s projects via the kubernetes/test-infra project, is that about right? 🤔 Not 100% sure I'm up to that but... heh... I might be on the right path.

Yes, the test infra will invoke the prow-presubmit-check target from the Makefile automatically; you don't need to worry about that part. So the short version of what you'll need to do is create a new make target that is added as one of the steps in prow-presubmit-check. You'll also need a target that installs goreleaser (i.e. go install github.com/goreleaser/goreleaser@latest -- see e.g. the $(MYGOBIN)/goimports target for reference), and the new local-build one will need to depend on that one. Hopefully that makes sense.

@patricknelson
Copy link
Contributor Author

Alright, thanks so much for the tips @KnVerey. I'll put these on my list to tackle in the next few days; if I end up stumped on that second item, instead of a PR I'll just submit an issue instead so it will (hopefully) be addressed by someone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants