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

enh: support direct updates #2335

Merged
merged 2 commits into from
Jul 24, 2024
Merged

enh: support direct updates #2335

merged 2 commits into from
Jul 24, 2024

Conversation

acpana
Copy link
Collaborator

@acpana acpana commented Jul 22, 2024

Adds update support for DataformRepository still in alpha!

@acpana acpana requested a review from justinsb July 22, 2024 23:07
@@ -197,6 +208,7 @@ func (a *Adapter) Update(ctx context.Context, u *unstructured.Unstructured) erro
return fmt.Errorf("converting DataformRepository spec to api: %w", mapCtx.Err())
}

resource.Name = a.fullyQualifiedName()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this could be part of the controller template

@acpana acpana requested a review from yuwenma July 23, 2024 17:22
Copy link
Collaborator

Choose a reason for hiding this comment

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

For full test suite, can you include all the configurable fields in the create.yaml? i.e. npmrcEnvironmentVariablesSecretVersion (unless it is not supported by the tf-based alpha)

Copy link
Collaborator Author

@acpana acpana Jul 23, 2024

Choose a reason for hiding this comment

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

No, those fields are not supported yet. Those are coming as part of https://github.com/GoogleCloudPlatform/k8s-config-connector/pull/2338/files ; as I work on that PR I will update the test suite on it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could add gitRemoteSettings here, I think. But we can do that in another PR

@acpana acpana requested a review from yuwenma July 23, 2024 22:57
@@ -188,7 +189,17 @@ func (a *Adapter) Create(ctx context.Context, u *unstructured.Unstructured) erro
func (a *Adapter) Update(ctx context.Context, u *unstructured.Unstructured) error {

updateMask := &fieldmaskpb.FieldMask{}
// TODO(acpana): handle updates

if a.desired.Spec.GitRemoteSettings != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Somewhere (maybe in an open PR) we have a helper to compute these, assuming we want the top-level fields.

Not a blocker

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could add gitRemoteSettings here, I think. But we can do that in another PR

Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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

The pull request process is described here

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

@google-oss-prow google-oss-prow bot merged commit 157a736 into master Jul 24, 2024
13 of 14 checks passed
@acpana acpana deleted the acpana/dir-dr-ctrl-2 branch July 26, 2024 19:23
@yuwenma yuwenma added this to the 1.121 milestone Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants