Skip to content

Conversation

@bennerv
Copy link
Member

@bennerv bennerv commented Jan 8, 2021

Things to note:

  1. Removed the dependency from dynamic on any document specific types, so it should be able to be called from anywhere that matches the arguments for NewValidator.
  2. OpenshiftCluster-validatedynamic still contains validation specific to the context of the RP. If some of this functionality needs to be moved elsewhere, it would [ideally] move into dynamic.go.
  3. OpenshiftCluster-validatedynamic utilizes the caching mechanism of the virtualnetworkclient in some of it's vnet specific validation. This can be changed or modified to use the raw virtualNetworkClient in cache.go instead of borrowing.

Which issue this PR addresses:

Refactor openshiftcluster-dynamicvalidator into an interface that allows some functions to be used outside of the context of the RP.

What this PR does / why we need it:

This should allow us to call validation methods from the context of the operator to expose them as conditions.

Test plan for issue:

Unit tests
E2E tests

Is there any documentation that needs to be updated for this PR?

No

@bennerv bennerv force-pushed the dynamic-validator-exploration branch from d499711 to f634b6f Compare January 13, 2021 17:45
@bennerv bennerv marked this pull request as ready for review January 13, 2021 21:49
@mjudeikis
Copy link
Contributor

I think this makes code more re-usable but it makes all the code exponentially more complicated and harder to read. Im ok with this but we just need to be more aware of things like this. Now it requires more experienced reviews, code is harder to change, any task touching this code will be much harder. Just saying :)

Copy link
Contributor

Choose a reason for hiding this comment

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

vnetId

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

json:"vnetId,omitempty"`

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@bennerv bennerv force-pushed the dynamic-validator-exploration branch from 7efdbaa to 7d91fe9 Compare January 19, 2021 15:25
@mjudeikis mjudeikis merged commit f2c083f into Azure:master Jan 20, 2021
Comment on lines +48 to +49
code string
typ string
Copy link
Contributor

Choose a reason for hiding this comment

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

Please raise follow-up to try to get this generalized so this would be set by called instead in the initiation

@bennerv bennerv deleted the dynamic-validator-exploration branch February 16, 2021 15:34
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.

2 participants