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

Implement Alibaba cloud SLS project #58

Merged
merged 13 commits into from
Apr 6, 2021
Merged

Conversation

zzxwill
Copy link
Collaborator

@zzxwill zzxwill commented Mar 10, 2021

Description of your changes

Support provision Alibaba Cloud SLS project, create examples directory
for official examples and update makefile

Fixes #

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

@wonderflow
Copy link
Contributor

@negz Hi, Nic, we're going to implement more cloud resources here, do you have any suggestion about the priority of cloud resources needed?

@negz
Copy link
Member

negz commented Mar 11, 2021

Hi, Nic, we're going to implement more cloud resources here

That's great news!

do you have any suggestion about the priority of cloud resources needed?

Why don't we start an issue on this repository to gather input from users - Slack would be a great place to ask the community for input on this.

@zzxwill zzxwill changed the title [WIP] Implement Alibaba cloud SLS project Implement Alibaba cloud SLS project Mar 16, 2021
Makefile Outdated Show resolved Hide resolved
apis/sls/v1alpha1/project_types.go Show resolved Hide resolved
apis/sls/v1alpha1/project_types.go Outdated Show resolved Hide resolved
apis/sls/v1alpha1/project_types.go Outdated Show resolved Hide resolved
pkg/clients/sls/project.go Outdated Show resolved Hide resolved
pkg/controller/sls/base.go Outdated Show resolved Hide resolved
pkg/controller/sls/base.go Outdated Show resolved Hide resolved
pkg/controller/sls/base.go Outdated Show resolved Hide resolved
pkg/controller/sls/project_controller.go Show resolved Hide resolved
}

func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.ExternalObservation, error) {
return BaseObserve(mg, e.client)
Copy link
Member

Choose a reason for hiding this comment

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

What is gained by breaking all this logic out into these separate Base functions, compared to just writing them inline?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please kindly refer to issue #63, in which I tried to explain the reason in details and wrote a few examples.

@negz
Copy link
Member

negz commented Mar 16, 2021

Thanks for working on this @zzxwill! Looks like a great start, but I would prefer if this managed resource was more in line with typical Crossplane conventions. If you strongly disagree with any we can discuss that and perhaps capture how provider-alibaba will diverge and why.

@negz negz mentioned this pull request Mar 16, 2021
2 tasks
hack/boilerplate.go.txt Outdated Show resolved Hide resolved
Support provision Alibaba Cloud SLS project, create `examples` directory
for official examples and update makefile

Signed-off-by: zzxwill <zzxwill@gmail.com>
Signed-off-by: zzxwill <zzxwill@gmail.com>
Moved copyright Year bump to crossplane-contrib#62 and examples folder changing to crossplane-contrib#61

Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com>
Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com>
Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com>
Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com>
Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

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

This is looking good @zzxwill, thank you. The main thing that still needs addressing is using meta.GetExternalName. I also wonder if we should agree on the pattern proposed in #63 before we merge this PR, but if it's urgent I could be convinced to approve while we figure out #63.

pkg/controller/sls/base.go Outdated Show resolved Hide resolved
pkg/controller/sls/base.go Outdated Show resolved Hide resolved
Copy link
Contributor

@hongchaodeng hongchaodeng left a comment

Choose a reason for hiding this comment

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

The code LGTM.

Please address @negz 's comments. @negz Please help approve when it is done :)

@wonderflow
Copy link
Contributor

HI @negz Can we merge it first and fix the rest in following PRs. We need it to be merged and show in demos.

@negz
Copy link
Member

negz commented Mar 26, 2021

HI @negz Can we merge it first and fix the rest in following PRs. We need it to be merged and show in demos.

No, sorry. I recommend building the provider from your branch if you need to use it for demos. I'm happy to merge once the remaining comments are addressed.

and wrap all errors

Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com>
@zzxwill
Copy link
Collaborator Author

zzxwill commented Apr 5, 2021

This is looking good @zzxwill, thank you. The main thing that still needs addressing is using meta.GetExternalName.
Done. Per your suggestion, I believe SLS project name spec.froProvider.name should be deleted and the managed resource name should be regarded as the SLS project name.

I also wonder if we should agree on the pattern proposed in #63 before we merge this PR, but if it's urgent I could be convinced to approve while we figure out #63.
I will add more some examples in #63 after I talked with my colleagues which will integrated provider-base base logics.

@zzxwill
Copy link
Collaborator Author

zzxwill commented Apr 5, 2021

@negz And I am so sorry for the late of addressing your newly two comments:) I was so busy with some other stuffs.

Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com>
@negz
Copy link
Member

negz commented Apr 5, 2021

No problem @zzxwill! We've been very busy too. :) Thanks for addressing my comments about using GetExternalName and pkg/errors. It seems like there are now only two things we need to sort out to merge this:

I prefer to not introduce the BaseObserve pattern in this PR until after we have a chance to discuss the context and approach in #63. How do you feel about updating this PR to use a 'normal' ExternalClient implementation for now, until we discuss further and agree?

Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com>
Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com>
@zzxwill
Copy link
Collaborator Author

zzxwill commented Apr 6, 2021

@negz Your suggestion is helpful for decomposing the problems. Addressed all of the two. Thanks help review it:)

Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com>
Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com>
Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com>
@zzxwill
Copy link
Collaborator Author

zzxwill commented Apr 6, 2021

Why don't we start an issue on this repository to gather input from users - Slack would be a great place to ask the community for input on this.

Create the issue #67, and I will talk to some potential users and contributors offline first.

Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

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

Excellent work @zzxwill - thank you!

@negz negz merged commit 0534f02 into crossplane-contrib:master Apr 6, 2021
@zzxwill zzxwill deleted the sls branch April 7, 2021 01:48
@zzxwill zzxwill restored the sls branch April 7, 2021 01:49
@zzxwill zzxwill mentioned this pull request Apr 25, 2021
2 tasks
@zzxwill zzxwill deleted the sls branch April 25, 2021 09:25
@zzxwill zzxwill mentioned this pull request Jul 20, 2021
6 tasks
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.

None yet

4 participants