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

feat: add ability to add ConfigMap checksum as resource annotations #4174

Closed
wants to merge 1 commit into from

Conversation

wyvernzora
Copy link
Contributor

@wyvernzora wyvernzora commented May 17, 2024

Note: draft change do not merge

One common practice that I have seen is adding the checksum of config map contents as annotations to workloads, so that when a config map is updated, the associated deployments can also get updated and bounced. This PR is an attempt to implement such functionality, though I am not sure about some aspects of this change and would appreciate some feedback.

Specific things that I'd like some feedback on:

  • Should we expose the checksum as a property on ConfigMap? For example cm.checksum. While the type of that property would need to be string, but in fact it needs to be Lazy that produces a string. I am not sure what the practice is for exposing a Lazy as a property on a construct.
  • Choosing to not expose the checksum outside the ConfigMap results in this awkward addChecksumTo() API. I'd appreciate any input on what would be a good API pattern for adding these checksum annotations.
  • I had to add a dependency on json-stable-stringify to make sure that hashes come out consistent. What are the opinions of this project on adding dependencies?

Signed-off-by: Denis Luchkin-Zhou <wyvernzora@gmail.com>
@wyvernzora wyvernzora changed the title Add ability to add ConfigMap checksum as resource annotations feat: add ability to add ConfigMap checksum as resource annotations May 17, 2024
@iliapolo iliapolo changed the base branch from k8s-29/main to k8s-30/main June 4, 2024 15:26
@iliapolo iliapolo added backport-to-k8s-28/main backport-to-k8s-29/main Backport a PR to the k8s-30 branch labels Jun 4, 2024
*/
public addChecksumTo(obj: base.Resource) {
obj.metadata.addAnnotation(
`checksum/${this.node.id}`,
Copy link
Member

@iliapolo iliapolo Sep 11, 2024

Choose a reason for hiding this comment

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

node.id is unique only in a given scope, node.addr will give an app unique id.

Suggested change
`checksum/${this.node.id}`,
`cdk8s.io/config-map.${this.node.addr}.checksum`,

},
});
const ns = new Namespace(chart, 'my-namespace', {});
cm.addChecksumTo(ns);
Copy link
Member

Choose a reason for hiding this comment

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

@iliapolo
Copy link
Member

@wyvernzora

Should we expose the checksum as a property on ConfigMap?

No need. At least not right now.

Choosing to not expose the checksum outside the ConfigMap results in this awkward addChecksumTo()

Agree. But its ok. Exposing lazy strings is also quite awkward.

I had to add a dependency on json-stable-stringify to make sure that hashes come out consistent. What are the opinions of this project on adding dependencies?

We try to avoid it, but also recognize sometimes it is needed. This seems like one of those times 👍

@iliapolo iliapolo added the response-requested Awaiting response from author label Sep 11, 2024
@iliapolo iliapolo changed the base branch from k8s-30/main to k8s-31/main September 11, 2024 11:54
@iliapolo iliapolo added backport-to-k8s-30/main Backport a PR to the k8s-30 branch and removed backport-to-k8s-28/main labels Sep 11, 2024
Copy link

This PR has not received a response in a while and will be closed soon. If you want to keep it open, please leave a comment below @mentioning a maintainer.

@github-actions github-actions bot added the closing-soon Issue/PR will be closing soon if no response is provided label Oct 11, 2024
@github-actions github-actions bot added closed-for-staleness Issue/PR was closed due to staleness and removed closing-soon Issue/PR will be closing soon if no response is provided labels Oct 18, 2024
@github-actions github-actions bot closed this Oct 18, 2024
@burner1024
Copy link

Please reconsider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-k8s-29/main Backport a PR to the k8s-30 branch backport-to-k8s-30/main Backport a PR to the k8s-30 branch closed-for-staleness Issue/PR was closed due to staleness response-requested Awaiting response from author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants