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

Include full state under status.atProvider #164

Merged
merged 3 commits into from
Apr 11, 2023
Merged

Include full state under status.atProvider #164

merged 3 commits into from
Apr 11, 2023

Conversation

turkenh
Copy link
Member

@turkenh turkenh commented Feb 24, 2023

Description of your changes

This PR changes schema generation to include the entire state under status.atProvider as proposed in the Observe Only design doc. In other words, status.atProvider will become superset of spec.forProvider.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

See this PR crossplane-contrib/provider-upjet-aws#571

@turkenh turkenh requested review from sergenyalcin and ulucinar and removed request for sergenyalcin February 24, 2023 14:11
@turkenh turkenh marked this pull request as ready for review February 24, 2023 14:21
@turkenh turkenh closed this Feb 24, 2023
@turkenh turkenh reopened this Feb 24, 2023
@turkenh turkenh mentioned this pull request Mar 2, 2023
3 tasks
@turkenh turkenh requested a review from lsviben March 2, 2023 09:05
Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thanks @turkenh. Left some comments for you to consider.

pkg/types/field.go Outdated Show resolved Hide resolved
@@ -277,6 +277,12 @@ func (r *resource) addParameterField(f *Field, field *types.Var) {
}

func (r *resource) addObservationField(f *Field, field *types.Var) {
for _, obsF := range r.obsFields {
if obsF.Name() == field.Name() {
// If the field is already added, we don't add it again.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do observation fields get added multiple times? Is it related to this code segment? The comment above that segment states that it prevents elimination of fields in the observation type. If we are unconditionally adding every field as an observation field, can we remove that segment? And if we did so, would we still need this duplication check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing that code segment and the check here didn't work OOB. Ended up with diffs like below:

Screen Shot 2023-04-05 at 17 21 26

I need to investigate further and remember where those previous additions are coming from.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment on where those previous additions are coming from.

Copy link
Collaborator

@ulucinar ulucinar Apr 11, 2023

Choose a reason for hiding this comment

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

Thank you @turkenh for giving it a try. I believe we can rename those types if desired in a future PR once we understand the implications as they are now shared between spec.forProvider and status.atProvider, when the status.atProvider becomes a superset.

Copy link
Member

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @turkenh LGTM! About @ulucinar's comment I have not a strong insight. But if you will continue to discussing on this, we may also wait his approval.

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thank you @turkenh, lgtm.

@turkenh turkenh merged commit 0132b23 into crossplane:main Apr 11, 2023
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