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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ce46d38
Include full state under status.atProvider
turkenh Feb 24, 2023
a0306ce
Do not add fields with no tag to status
turkenh Apr 5, 2023
e376ed8
Add comment for previously added observation types
turkenh Apr 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions pkg/types/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,14 @@ 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 •
edited
Loading

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.

// Some nested types could have been previously added as an
// observation type while building their schema: /~https://github.com/upbound/upjet/blob/b89baca4ae24c8fbd8eb403c353ca18916093e5e/pkg/types/builder.go#L206
return
}
}
r.obsFields = append(r.obsFields, field)
r.obsTags = append(r.obsTags, fmt.Sprintf(`json:"%s" tf:"%s"`, f.JSONTag, f.TFTag))
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/types/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ func TestBuild(t *testing.T) {
},
want: want{
forProvider: `type example.Parameters struct{Enable *bool "json:\"enable,omitempty\" tf:\"enable,omitempty\""; ID *int64 "json:\"id\" tf:\"id,omitempty\""; Name *string "json:\"name\" tf:\"name,omitempty\""}`,
atProvider: `type example.Observation struct{Config *string "json:\"config,omitempty\" tf:\"config,omitempty\""; Value *float64 "json:\"value,omitempty\" tf:\"value,omitempty\""}`,
atProvider: `type example.Observation struct{Config *string "json:\"config,omitempty\" tf:\"config,omitempty\""; Enable *bool "json:\"enable,omitempty\" tf:\"enable,omitempty\""; ID *int64 "json:\"id,omitempty\" tf:\"id,omitempty\""; Name *string "json:\"name,omitempty\" tf:\"name,omitempty\""; Value *float64 "json:\"value,omitempty\" tf:\"value,omitempty\""}`,
},
},
"Resource_Types": {
Expand Down Expand Up @@ -283,7 +283,7 @@ func TestBuild(t *testing.T) {
},
want: want{
forProvider: `type example.Parameters struct{List []*string "json:\"list\" tf:\"list,omitempty\""; ResourceIn map[string]example.ResourceInParameters "json:\"resourceIn\" tf:\"resource_in,omitempty\""}`,
atProvider: `type example.Observation struct{ResourceOut map[string]example.ResourceOutObservation "json:\"resourceOut,omitempty\" tf:\"resource_out,omitempty\""}`,
atProvider: `type example.Observation struct{List []*string "json:\"list,omitempty\" tf:\"list,omitempty\""; ResourceIn map[string]example.ResourceInParameters "json:\"resourceIn,omitempty\" tf:\"resource_in,omitempty\""; ResourceOut map[string]example.ResourceOutObservation "json:\"resourceOut,omitempty\" tf:\"resource_out,omitempty\""}`,
},
},
"Sensitive_Fields": {
Expand Down Expand Up @@ -355,7 +355,7 @@ func TestBuild(t *testing.T) {
},
want: want{
forProvider: `type example.Parameters struct{Name *string "json:\"name\" tf:\"name,omitempty\""; ReferenceID *string "json:\"referenceId,omitempty\" tf:\"reference_id,omitempty\""; ExternalResourceID *github.com/crossplane/crossplane-runtime/apis/common/v1.Reference "json:\"externalResourceId,omitempty\" tf:\"-\""; ReferenceIDSelector *github.com/crossplane/crossplane-runtime/apis/common/v1.Selector "json:\"referenceIdSelector,omitempty\" tf:\"-\""}`,
atProvider: `type example.Observation struct{}`,
atProvider: `type example.Observation struct{Name *string "json:\"name,omitempty\" tf:\"name,omitempty\""; ReferenceID *string "json:\"referenceId,omitempty\" tf:\"reference_id,omitempty\""}`,
},
},
"Invalid_Schema_Type": {
Expand Down
22 changes: 19 additions & 3 deletions pkg/types/field.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,10 +204,18 @@ func (f *Field) AddToResource(g *Builder, r *resource, typeNames *TypeNames) {
}

field := types.NewField(token.NoPos, g.Package, f.FieldNameCamel, f.FieldType, false)
switch {
case IsObservation(f.Schema):

// Note(turkenh): We want atProvider to be a superset of forProvider, so
// we always add the field as an observation field and then add it as a
// parameter field if it's not an observation (only) field, i.e. parameter.
//
// We do this only if tf tag is not set to "-" because otherwise it won't
// be populated from the tfstate. We typically set tf tag to "-" for
// sensitive fields which were replaced with secretKeyRefs.
if f.TFTag != "-" {
r.addObservationField(f, field)
default:
}
if !IsObservation(f.Schema) {
if f.AsBlocksMode {
f.TFTag = strings.TrimSuffix(f.TFTag, ",omitempty")
}
Expand All @@ -219,6 +227,14 @@ func (f *Field) AddToResource(g *Builder, r *resource, typeNames *TypeNames) {
}

g.comments.AddFieldComment(typeNames.ParameterTypeName, f.FieldNameCamel, f.Comment.Build())
// Note(turkenh): We don't want reference resolver to be generated for
// fields under status.atProvider. So, we don't want reference comments to
// be added, hence we are unsetting reference on the field comment just
// before adding it as an observation field.
f.Comment.Reference = config.Reference{}
// Note(turkenh): We don't need required/optional markers for observation
// fields.
f.Comment.Required = nil
g.comments.AddFieldComment(typeNames.ObservationTypeName, f.FieldNameCamel, f.Comment.Build())
}

Expand Down