Skip to content

Conversation

@nicksnyder
Copy link
Contributor

@nicksnyder nicksnyder commented Aug 22, 2022

A customer reported lint warnings in the output yaml from kube yaml plugin for IntellilJ because annotation: keys were set with no value.

This PR updates the chart to not emit the annotations key if no values are going to be set.

I am not sure how to enforce this in unit tests. helm unittest seems to only be capable of checking values, not the existence of keys, so tests can't tell the difference between the annotations: key not being present or simply empty.

Alternatively, we could just require that every entity has a description annotation.

Checklist

Test plan

Ran helm template ./charts/sourcegraph and ensured all annotations: keys had values.

@nicksnyder nicksnyder requested a review from michaellzc August 22, 2022 19:32
Copy link
Member

@michaellzc michaellzc left a comment

Choose a reason for hiding this comment

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

lgtm

if you think unit test is necessary, we can consider using matchSnapshot https://github.com/vbehar/helm3-unittest/blob/master/DOCUMENT.md#assertion-types

CI somehow is not happy with this PR, would you take a look?

@nicksnyder nicksnyder enabled auto-merge (squash) August 22, 2022 23:00
@nicksnyder nicksnyder disabled auto-merge August 22, 2022 23:00
@nicksnyder nicksnyder changed the title Don't produce yaml with empty annotations Don't include annotations: key when there are no annotations Aug 22, 2022
@nicksnyder nicksnyder enabled auto-merge (squash) August 22, 2022 23:00
@nicksnyder
Copy link
Contributor Author

@michaellzc are you able to give me an official approve so I can merge or is there someone else I should request?

@nicksnyder nicksnyder merged commit 28664ce into main Aug 22, 2022
@nicksnyder nicksnyder deleted the ns/noannotations branch August 22, 2022 23:35
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.

3 participants