Skip to content

Conversation

@abhinavdahiya
Copy link
Contributor

@abhinavdahiya abhinavdahiya commented Oct 4, 2018

  1. update cvoconfig's clusterid

Making CLusterID a string type allows makes the config better in
terms of human readble.
Added the UnmarshalJSON for string CLusterID with validations that
ensure that uuid type.
This does not prevent users from setting wrong uuid type, but CRDs
don't allow high level validation server side. CVO will not use invalid
uuid.

  1. add overrides per component basis

component override allow user to make one of the CVO's owned component
unmanaged.
More overrides like JSON merge patch can be added later.

Making CLusterID a string type allows makes the config better in
terms of human readble.

Added the UnmarshalJSON for string CLusterID with validations that
ensure that uuid type.

This does not prevent users from setting wrong uuid type, but CRDs
don't allow high level validation server side. CVO will not use invalid
uuid.
component override allow user to make one of the CVO's owned component
unmanaged.

More overrides like JSON merge patch can be added later.
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 4, 2018
@abhinavdahiya
Copy link
Contributor Author

/retest

return fmt.Errorf("invalid ClusterID %q, must be an RFC4122-variant UUID: found %s", strid, uid.Variant())
}
if uid.Version() != 4 {
return fmt.Errorf("Invalid ClusterID %q, must be a version-4 UUID: found %s", strid, uid.Version())
Copy link
Member

Choose a reason for hiding this comment

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

Wait, why do we care about the UUID version?

Copy link
Contributor Author

@abhinavdahiya abhinavdahiya Oct 4, 2018

Choose a reason for hiding this comment

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

cincinnati cares about it. And CVO needs to only use that specific version of uuid to successfully talk to cincinnati

Copy link
Contributor

Choose a reason for hiding this comment

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

We are banking on the fact that V4 UUIDs are random so that we can key on this when doing rate-limited updates.

if ov.Kind == kind &&
(namespace == "" || ov.Namespace == namespace) && // cluster-scoped objects don't have namespace.
ov.Name == name {
return overrides[idx], true
Copy link
Member

Choose a reason for hiding this comment

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

Why overrides[idx] instead of ov?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't like using the value from range for anything other than reading....

@abhinavdahiya
Copy link
Contributor Author

/retest

@crawford
Copy link
Contributor

crawford commented Oct 4, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, crawford

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2018
@openshift-merge-robot openshift-merge-robot merged commit a5a76d5 into openshift:master Oct 4, 2018
@abhinavdahiya abhinavdahiya deleted the allow_overrides branch October 4, 2018 16:27
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. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants