Skip to content

Simplify config by merge CloudProviderType with ApplicationKind #3402

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

Jump to bottom
Merged
pipecd-bot merged 1 commit into from
Mar 17, 2022
Merged

Simplify config by merge CloudProviderType with ApplicationKind #3402

pipecd-bot merged 1 commit into from
Mar 17, 2022

Conversation

@nghialv
Copy link
Member

@nghialv nghialv commented Mar 17, 2022 •
edited
Loading

What this PR does / why we need it:

This PR deletes CloudProviderType and uses ApplicationKind instead for CloudProvider configuration.
This allows us to remove unneeded syncing between ApplicationKind and CloudProviderType.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

@pipecd-bot
Copy link
Collaborator

pipecd-bot commented Mar 17, 2022

COVERAGE

Code coverage for golang is 35.51%. This pull request increases coverage by 0.02%.

File Function Base Head Diff
pkg/model/cloudprovider.go CloudProviderType.String 0.00% -- +-0.00%
pkg/model/deployment.go Deployment.CloudProviderType 0.00% -- +-0.00%

@knanao
Copy link
Member

knanao commented Mar 17, 2022

Nice catch🙌
/lgtm

khanhtc1202
khanhtc1202 reviewed Mar 17, 2022
View reviewed changes
pkg/config/piped.go
Type model.CloudProviderType `json:"type"`
Config json.RawMessage `json:"config"`
Name string `json:"name"`
Type string `json:"type"`
Copy link
Member

@khanhtc1202 khanhtc1202 Mar 17, 2022

Choose a reason for hiding this comment

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

nits, should we do the same thing we did at L338, instead of using type string, should we use type model.ApplicationKind?

Copy link
Member Author

@nghialv nghialv Mar 17, 2022

Choose a reason for hiding this comment

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

I think it will not work since model.ApplicationKind is int32 not a string.
Or we have to add a custom Marshal function.

Copy link
Member

@khanhtc1202 khanhtc1202 Mar 17, 2022

Choose a reason for hiding this comment

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

Yes, I mean to that since currently, we convert it at L360. But you're right, add a custom marshal function is not worth doing here. Thanks for clarification 🙏

@khanhtc1202
Copy link
Member

khanhtc1202 commented Mar 17, 2022

Nice improvement 🙆‍♂️
/approve

@pipecd-bot
Copy link
Collaborator

pipecd-bot commented Mar 17, 2022

APPROVE

This pull request is APPROVED by khanhtc1202.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

@pipecd-bot pipecd-bot merged commit c2daab2 into master Mar 17, 2022
@pipecd-bot pipecd-bot deleted the config branch March 17, 2022 03:04
This was referenced Mar 17, 2022
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@khanhtc1202 khanhtc1202 khanhtc1202 left review comments

@knanao knanao Awaiting requested review from knanao

Assignees

No one assigned

Labels

approved area/piped lgtm release-note-none size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nghialv @pipecd-bot @knanao @khanhtc1202