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

Add GoReleaser #1549

Merged
merged 5 commits into from
Apr 24, 2021
Merged

Add GoReleaser #1549

merged 5 commits into from
Apr 24, 2021

Conversation

lucacome
Copy link
Member

@lucacome lucacome commented Apr 21, 2021

This will get the wrong tag until #1544 is merged.

Removed edge from the version name in the Makefile, it will now be something like v1.11.1-SNAPSHOT-00f0053 for local builds (same that GoReleaser uses for edge), so it will remove the need to update the version in the Makefile.
(When we make a stable release, GoReleaser will use v1.11.1)

GoReleaser builds binaries for:

  • amd64
  • arm
  • arm64
  • ppc64le
  • s390x

and we push Docker images for the same architectures.

Addresses #944, #974 and #1310

@lucacome lucacome self-assigned this Apr 21, 2021
@lucacome lucacome requested review from pleshakov and soneillf5 and removed request for pleshakov April 21, 2021 03:52
@github-actions github-actions bot added the chore Pull requests for routine tasks label Apr 21, 2021
@lucacome lucacome force-pushed the chore/goreleaser branch 5 times, most recently from f9217bb to 3c66548 Compare April 21, 2021 16:23
@@ -46,6 +45,8 @@ jobs:
steps:
- name: Checkout Repository
uses: actions/checkout@v2
with:
fetch-depth: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we do this? does it break otherwise ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, the docs for GoReleaser say:

Note the fetch-depth: 0 option on the Checkout workflow step. It is required for GoReleaser to work properly. Without that, GoReleaser might fail or behave incorrectly.

this is the description of the parameter:

Number of commits to fetch. 0 indicates all history for all branches and tags.

Copy link
Contributor

@soneillf5 soneillf5 left a comment

Choose a reason for hiding this comment

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

Really cool! It's so different from the stuff I learnt when starting go about targeting all the different architectures.

@@ -53,3 +53,5 @@ hack/controller-gen-*
docs-web/.netlify/state.json
site/
venv/

dist/
Copy link
Contributor

Choose a reason for hiding this comment

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

cool!

- GO111MODULE=on
before:
hooks:
- go mod tidy
Copy link
Contributor

Choose a reason for hiding this comment

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

are tidy and verify commands really necessary?
if they are expected to change the repo contents, would it be better if our pipeline had a check in a separate stage - to check that the repo files do not change after tidy and verify?

Copy link
Member Author

Choose a reason for hiding this comment

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

they're not expected to change the repository's content, it's just a check to make sure everything is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

go mod tidy exits with 0 status code even if it updates the go.mod. so it will silently fix the files.

@@ -272,7 +272,7 @@ RUN mkdir -p /var/lib/nginx /etc/nginx/secrets /etc/nginx/stream-conf.d \
&& chown -R nginx:0 /etc/nginx /var/cache/nginx /var/lib/nginx \
&& rm -f /etc/nginx/conf.d/* /etc/apt/apt.conf.d/90nginx /etc/apt/sources.list.d/nginx-plus.list

COPY internal/configs/version1/nginx$PLUS.ingress.tmpl \
COPY --chown=nginx:0 internal/configs/version1/nginx$PLUS.ingress.tmpl \
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this change necessary?
for the template files, it is ok if root owns it, because the IC only need to read it and only to read it on the start.

Copy link
Member Author

Choose a reason for hiding this comment

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

We had a problem not long ago with permissions and having nginx be the owner of the files will fix that. Since we set nginx as the user and run the binary as nginx user, it makes sense that nginx owns the files.

Copy link
Contributor

Choose a reason for hiding this comment

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

for the files that the IC or NGINX workers can change, it makes sense. But for the files that IC or NGINX only need to read, I think it makes sense to be owned by root and allow read/execute on those, to follow the principle of least privilege. For example, as NGINX binary:

ls -la /usr/sbin/nginx
-rwxr-xr-x 1 root root 1698544 Nov 26 13:00 /usr/sbin/nginx

Copy link
Member Author

Choose a reason for hiding this comment

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

isn't it least privilege if the user is nginx instead of root?

build/Dockerfile Show resolved Hide resolved
cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the documentation Pull requests/issues for documentation label Apr 23, 2021
@lucacome lucacome merged commit 40db5b5 into master Apr 24, 2021
@lucacome lucacome deleted the chore/goreleaser branch April 24, 2021 00:41
@lucacome lucacome removed the documentation Pull requests/issues for documentation label Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Pull requests for routine tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants