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

Support configure resources #48

Merged
merged 2 commits into from
Mar 18, 2021

Conversation

clyang82
Copy link
Contributor

Enable configure resources for each component.

Signed-off-by: clyang82 [email protected]

@clyang82 clyang82 force-pushed the support_resources branch 4 times, most recently from fdc217e to 4c513c8 Compare March 16, 2021 15:38
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM. However, I have long term concerns.

First of all these reviews are getting challenging. We need to define clear ownership of this component and what we would like to achieve. We need to build context and knowledge.

Second, jsonnet files for optional attributes are getting out of control. They are cumbersome and not easy to digest. We should come up with a better pattern.

And last we are getting reckless with the API, we just keep adding things to CRD. The existing API already seems archaic comparing our recent configuration. Moreover, we don't think about the future that we plan, we might need to rewrite the whole CRD to support other signals.

Adding my thoughts here to make them visible, these don't particularly concern things introduce in this PR.

@kakkoyun kakkoyun merged commit 82f0859 into observatorium:master Mar 18, 2021
@bwplotka
Copy link
Member

bwplotka commented Mar 18, 2021

Agree with @kakkoyun. We should find a way to define way forward that does not block experimentation but does not trash API CRD that suppose to serve some purpose

@bwplotka
Copy link
Member

cc @paulfantom

@bwplotka
Copy link
Member

Adde #57 @kakkoyun

haoqing0110 referenced this pull request in stolostron/observatorium-operator Apr 7, 2021
Currently, merging to master triggers the tagged-master job, because it
seems that CircleCI filters are ORed. This causes the Makefile to try to
tag a docker container with a trailing `:` since the Git tag is emtpy.
This commit changes the logic so that the tagged-master job is only
triggered on a valid SemVer tag.

Signed-off-by: Lucas Servén Marín <[email protected]>
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