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 way to provide annotations to ingress/route resources #206

Merged
merged 5 commits into from
Jun 6, 2021

Conversation

LCaparelli
Copy link
Member

@LCaparelli LCaparelli commented Jun 2, 2021

Fix #205
Relates to #200

Signed-off-by: Lucas Caparelli lucas.caparelli112@gmail.com

Signed-off-by: Lucas Caparelli <lucas.caparelli112@gmail.com>
Signed-off-by: Lucas Caparelli <lucas.caparelli112@gmail.com>
@@ -25,7 +25,7 @@ jobs:
with:
# Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version.
version: latest
args: --enable=golint --timeout=2m
args: --enable=revive --timeout=2m --skip-files=pkg/test/client.go,pkg/framework/kind/kinds.go,controllers/nexus/resource/validation/defaults.go
Copy link
Member Author

Choose a reason for hiding this comment

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

golint was deprecated, hence the change to revive.

fake, which we use on pkg/test/client.go was also deprecated, so I added it to the skip list until a more definitive solution.

The other two files are full of constants and vars which are sufficiently described by their names. Adding comments would render pearls like:

// NexusCommunityImage is the community image of Nexus
NexusCommunityImage = "docker.io/sonatype/nexus3"

Not very productive, so I added them to the skip list.

@LCaparelli
Copy link
Member Author

Dunno what's up with the tests, ran fine locally. I'll have a look later

@ricardozanini
Copy link
Member

Maybe this?

/home/runner/work/nexus-operator/nexus-operator/testbin/setup-envtest.sh: line 1: 404:: command not found
/bin/bash: fetch_envtest_tools: command not found
/bin/bash: setup_envtest_env: command not found

Signed-off-by: Lucas Caparelli <lucas.caparelli112@gmail.com>
Signed-off-by: Lucas Caparelli <lucas.caparelli112@gmail.com>
@LCaparelli LCaparelli force-pushed the 205-ingress-annotations branch from 5347018 to ff89cfd Compare June 5, 2021 21:47
@LCaparelli
Copy link
Member Author

Turns out the Makefile recipe we were using fetched the script from the master branch, but that script was broken into separate files, so it got a 404 when trying to download it. I changed the recipe to fetch the script from controller-runtime v0.6.3, which should work fine with our current version of the SDK.

Signed-off-by: Lucas Caparelli <lucas.caparelli112@gmail.com>
Comment on lines +42 to +43
#sed -i "s,#\!.*,#\!\/bin\/bash,g" ${ENVTEST_ASSETS_DIR}/setup-envtest.sh
#sed -i "/pipefail/d" ${ENVTEST_ASSETS_DIR}/setup-envtest.sh
Copy link
Member

Choose a reason for hiding this comment

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

Afraid to remove them completely? 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I didn't want to remove since it was created by the SDK, should cause less confusion when migrating to newer versions (i.e., the code block will be there, though commented out vs. not having the lines at all)

Copy link
Member

Choose a reason for hiding this comment

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

Oh no, I added this to fix an error in their script. I believe I should've sent a PR instead. We can remove these lines.

@ricardozanini
Copy link
Member

I'm gonna merge to clear the way for the others PRs, we can remove the sed comments later.

@ricardozanini ricardozanini merged commit 5eb61f1 into m88i:main Jun 6, 2021
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.

Provide a way to specify user-defined ingress annotations
2 participants