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 local setup issues #194

Merged
merged 4 commits into from
Nov 26, 2024
Merged

Conversation

ialidzhikov
Copy link
Member

How to categorize this PR?

/area dev-productivity
/kind bug

What this PR does / why we need it:
This PR performs the following changes from the registry-cache extension which are applicable also to this extension:

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:
N/A

Release note:

An issue causing `make extension-up` to fail to patch the ControllerDeployment is now mitigated.
An issue causing `make extension-up` to do NOT generate a new tag for local source code changes is now fixed.

@gardener-prow gardener-prow bot added area/dev-productivity Developer productivity related (how to improve development) kind/bug Bug labels Nov 11, 2024
@gardener-prow gardener-prow bot requested review from Kostov6 and plkokanov November 11, 2024 09:11
@gardener-prow gardener-prow bot added cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 11, 2024
skaffold.yaml Outdated
envTemplate:
template: "{{.EXTENSION_VERSION}}"
- name: sha
inputDigest: {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Iirc this is only necessary for the ControllerDeployment because we only use the image name and tag - the digest is not available from the $SKAFFOLD_IMAGE env variable or any other variable

In other cases skaffold includes the digest in the image by default when the gitCommit tagPolicy is used - name:tag@sha256:garden.local.gardener.cloud:5001..., that is why in g/g the following is used:
https://github.com/gardener/gardener/blob/01391f78c30a37968fceafd885f0637e689f1909/skaffold.yaml#L1704-L1706

Wdyt about keeping it in line with how it is done in g/g?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure that it will work with your suggestion. I haven't tried and I won't have capacity this week to try it out locally.
I am not sure if https://github.com/gardener/gardener/blob/01391f78c30a37968fceafd885f0637e689f1909/skaffold.yaml#L1696-L1706 will help in the case you have changes locally which are not committed. With inputDigest I want to cover the case where you change the code (without local commit) and skaffold generates a new tag.

Copy link
Collaborator

@plkokanov plkokanov Nov 20, 2024

Choose a reason for hiding this comment

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

Yep, having changes locally which are not committed is covered. From the tagging docs

When images are pushed, their immutable digest is available. Skaffold then references images both by tag and digest. Something like image:tag@sha256:abacabac.... Using both the tag and the digest seems superfluous but it guarantees immutability and helps users quickly see which version of the image is used.

I also tested it locally. Without any file changes I got the following image for the admission deployment:

image: garden.local.gardener.cloud:5001/local-skaffold_gardener-extension-shoot-rsyslog-relp-admission:v0.7.0-dev-6b6e603-dirty@sha256:f68ae005709b2b217d02d75f50612f2c39c8bf5250909a30f9867f395e591f39

After adding changes to a file, part of the admission, and executing make extensions-up (without committing the changes in git), I got an image with the same tag, but a different sha digest:

image: garden.local.gardener.cloud:5001/local-skaffold_gardener-extension-shoot-rsyslog-relp-admission:v0.7.0-dev-6b6e603-dirty@sha256:852ce3df6d2c7f91b861f42edcefb1f24651e851eb72eef2bcb103f31cf37581

This is how it used to work before. The actual reason for the issue was that we switched to using https://github.com/gardener/gardener-extension-shoot-rsyslog-relp/blob/main/hack/generate-kustomize-patch-controllerdeployment-shoot-rsyslog-relp.sh to generate the ControllerDeployment as the SKAFFOLD_IMAGE env variable does not contain the sha digest that would differ depending on local code changes (it only contains the tag). That's why we need to use inputDigest when building the extension image for the ControllerDeployment to manually add this digest to the tag.

Anyway, I was initially confused that there were no problems some time ago when making local code changes, and by skaffold in gardener where inputDigest was only used for provider-local.

Adding a comment for the extension config here to explain this would be useful. Maybe something in the lines of

inputDigest is used to inject a digest of the artifact source into the built image tag and therefore into the SKAFFOLD_IMAGE environment variable which is used when generating the corresponding ControllerDeployment

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed 3816113 to address your suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI @rrhubenov as you wanted to the the same changes to the lakom extension.

@gardener-prow gardener-prow bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 20, 2024
@gardener-prow gardener-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2024
@plkokanov
Copy link
Collaborator

/lgtm
/approve

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2024
Copy link

gardener-prow bot commented Nov 26, 2024

LGTM label has been added.

Git tree hash: 75f363104025bd0734d939cbaf63862d6aad3bcb

Copy link

gardener-prow bot commented Nov 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: plkokanov

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

@gardener-prow gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2024
@gardener-prow gardener-prow bot merged commit 13e26b9 into gardener:main Nov 26, 2024
7 checks passed
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. area/dev-productivity Developer productivity related (how to improve development) cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/bug Bug lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants