Skip to content

Conversation

@mjudeikis
Copy link
Contributor

Which issue this PR addresses:

Fixes our ability to serve old cluster using new api

Must merge after #1653

What this PR does / why we need it:

Currently if have a cluster created with old api and we will be doing operations on it using new API this will happen:

  1. Old cluster document is read from database
  2. Customer PUT/PATCH paylaod is layed over the the document
  3. Static validation runs and fails on ENUM values being "" in the document

With this we making sure that:

  1. After adminPutOrPatch cluster document defaults values.
  2. All new creates with OLD API will get default values injected into document as per old API fields (features from NEW api disabled by default in if using OLD API).
  3. Fields will get into database but converters will not return them to the user if using old API.

Test plan for issue:

Unit tests

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

No

@troy0820 troy0820 added priority-medium Medium priority issue or pull request ready-for-review size-medium Size medium labels Aug 4, 2021
@troy0820
Copy link
Contributor

troy0820 commented Aug 4, 2021

/azp run e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

Choose a reason for hiding this comment

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

fmt.Errorf -> cmp.Diff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/ // can't use cmp due to cycle imports

Copy link
Contributor

Choose a reason for hiding this comment

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

Should work if you import from github.com/google/go-cmp/cmp directly instead of using our wrapper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I want to do this. This is bigger evil in my book :)

@mjudeikis mjudeikis added priority-high High priority issue or pull request and removed priority-medium Medium priority issue or pull request labels Aug 6, 2021
Copy link
Contributor

Choose a reason for hiding this comment

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

Should work if you import from github.com/google/go-cmp/cmp directly instead of using our wrapper.

Copy link
Contributor

@m1kola m1kola left a comment

Choose a reason for hiding this comment

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

Just few nits. Looks good otherwise.

@mjudeikis mjudeikis mentioned this pull request Aug 11, 2021
@mjudeikis mjudeikis merged commit 5efde46 into Azure:master Aug 11, 2021
ross-bryan pushed a commit to ross-bryan/ARO-RP that referenced this pull request Sep 24, 2021
ross-bryan pushed a commit to ross-bryan/ARO-RP that referenced this pull request Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority-high High priority issue or pull request ready-for-review size-medium Size medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants