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 robot chart in app CRD #81

Closed

Conversation

oliver-goetz
Copy link
Contributor

@ensonic there was one more little inconsistency between apps-crd.yaml and the go structure I did not find in the first place. It's just one letter, but in the end it removes all the robot charts from App CRs.

@google-cla google-cla bot added the cla: yes cla signed label Oct 4, 2021
@@ -36,7 +36,7 @@ spec:
type: string
inline:
type: string
robots:
robot:
Copy link
Contributor

Choose a reason for hiding this comment

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

In line 94 below it is also robots:

Copy link
Contributor

Choose a reason for hiding this comment

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

I think robots (plural) is correct and also what we actually use everywhere

Copy link
Contributor Author

@oliver-goetz oliver-goetz Oct 4, 2021

Choose a reason for hiding this comment

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

Prometheus for example calls it prometheus-robot, if I understand the underlying bazel function correctly.
And the go type calls it robot too. That's why we call it robot in all our app charts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use robot in App CRs since the beginning and it would be more effort to change it here.
In the end there was no other way to use the name of the Go structure and we just did not notice the inconsistency with App CRD specification from app-crd.yaml because CRDs in v1beta1 preserved unknown fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

RIght, this is for the app! Makes sense.

@copybara-service copybara-service bot closed this in 9429adb Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes cla signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants