Make platformProviders works as piped deployment provider #3815

khanhtc1202 · 2022-07-21T06:00:26Z

What this PR does / why we need it:

This PR adds logic to make PlatformProviders specified via piped configuration work as the current CloudProviders. The CloudProviders configuration is marked as deprecated and will be removed after a few releases.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Enable PlatformProviders as piped configuration

pkg/app/piped/cmd/piped/piped.go

nghialv · 2022-07-21T06:55:17Z

pkg/config/piped.go

+func (s *PipedSpec) HasPlatformProvider(name string, t model.ApplicationKind) bool {
+	requiredProviderType := t.CompatiblePlatformProviderType()
+	for _, cp := range s.PlatformProviders {
+		if cp.Name == name && cp.Type == requiredProviderType {
+			return true
 		}
-		if cp.Type != requiredProviderType {
-			continue
+	}
+	for _, pp := range s.CloudProviders {
+		if pp.Name == name && pp.Type == requiredProviderType {
+			return true
 		}
-		return true
 	}
 	return false
 }


To avoid duplication how about removing this function and use FindCloudProvider instead?

addressed by f413a7e

nghialv · 2022-07-21T06:59:26Z

pkg/config/piped.go

+	for _, p := range s.PlatformProviders {
+		if p.Name != name {
+			continue
+		}
+		if p.Type != requiredProviderType {
+			continue
+		}
+		return p, true
+	}
 	for _, p := range s.CloudProviders {
 		if p.Name != name {
 			continue


I feel that instead of keeping both fields as this, converting all configuration ways to use only a single PlatformProviders field could be easier to maintain.
I mean we can remove the CloudProviders field immediately from the Piped config and customizing the Unmarhal function to allow accepting both cloudProviders and platformProviders specifications in YAML.

And by that way, I guess that we can remove a lot of duplications that are being added via this PR.

Nice idea, lets me try this 👍

addressed by 63d4b7e 🙏

nghialv · 2022-07-22T01:42:30Z

pkg/app/piped/cmd/piped/piped.go

-			Version:        version.Get().Version,
-			Config:         string(maskedCfg),
-			Repositories:   repos,
-			CloudProviders: make([]*model.Piped_CloudProvider, 0, len(cfg.CloudProviders)),


Is it ok to remove this field in gRPC?
As far as I see only CloudProvider is being used in the implementation of that RPC.
https://github.com/pipe-cd/pipecd/blob/master/pkg/app/server/grpcapi/piped_api.go#L167-L194

Nice catch, the current implementation pass from the test because I updated the ReportPipedMetaRequest previously (in here).

The API update for that is included by another PR

--- a/pkg/app/server/grpcapi/piped_api.go +++ b/pkg/app/server/grpcapi/piped_api.go @@ -69,7 +69,7 @@ type pipedApiDeploymentChainStore interface { type pipedApiPipedStore interface { Get(ctx context.Context, id string) (*model.Piped, error) - UpdateMetadata(ctx context.Context, id, version, config string, cps []*model.Piped_CloudProvider, repos []*model.ApplicationGitRepository, se *model.Piped_SecretEncryption, startedAt int64) error + UpdateMetadata(ctx context.Context, id, version, config string, pps []*model.Piped_PlatformProvider, repos []*model.ApplicationGitRepository, se *model.Piped_SecretEncryption, startedAt int64) error } type pipedApiEventStore interface { @@ -176,7 +176,7 @@ func (a *PipedAPI) ReportPipedMeta(ctx context.Context, req *pipedservice.Report pipedID, req.Version, req.Config, - req.CloudProviders, + req.PlatformProviders, req.Repositories, req.SecretEncryption, now,

but you're right, maybe we should include the change for that in this PR as well 🤔

maybe we should include the change for that in this PR as well

Yes, that is better. Or the PR for that API change should be merged before this PR.

Okay, lets me revise this. Thank you 🙏

addressed by 0272ba2

pkg/app/piped/executor/kubernetes/rollback.go

pkg/app/server/grpcapi/piped_api.go

nghialv · 2022-07-25T04:18:51Z

pkg/config/piped.go

+// HasPlatformProvider checks whether the given provider is configured or not.
+func (s *PipedSpec) HasPlatformProvider(name string, t model.ApplicationKind) bool {
+	_, contains := s.FindPlatformProvider(name, t)
+	return contains


khanhtc1202 · 2022-07-25T04:21:07Z

/hold
Hold until PR #3817 be merged ⏱️

nghialv · 2022-07-25T04:37:57Z

pkg/config/piped.go

+func (s *PipedSpec) UnmarshalJSON(data []byte) error {
+	ps := pipedSpec{}
+	if err := json.Unmarshal(data, &ps); err != nil {
+		return err
+	}
+
+	s.ProjectID = ps.ProjectID
+	s.PipedID = ps.PipedID
+	s.PipedKeyFile = ps.PipedKeyFile
+	s.PipedKeyData = ps.PipedKeyData
+	s.Name = ps.Name
+	s.APIAddress = ps.APIAddress
+	s.WebAddress = ps.WebAddress
+	s.SyncInterval = ps.SyncInterval
+	s.AppConfigSyncInterval = ps.AppConfigSyncInterval
+	s.Git = ps.Git
+	s.Repositories = ps.Repositories
+	s.ChartRegistries = ps.ChartRegistries
+	s.ChartRepositories = ps.ChartRepositories
+	// Add all CloudProviders configuration as PlatformProviders configuration.
+	s.PlatformProviders = append(s.PlatformProviders, ps.PlatformProviders...)
+	s.PlatformProviders = append(s.PlatformProviders, ps.CloudProviders...)
+	s.AnalysisProviders = ps.AnalysisProviders
+	s.Notifications = ps.Notifications
+	s.SecretManagement = ps.SecretManagement
+	s.EventWatcher = ps.EventWatcher
+	s.AppSelector = ps.AppSelector
+
+	return nil
+}


We should avoid this manual copy.
How about this?

Suggested change

func (s *PipedSpec) UnmarshalJSON(data []byte) error {

ps := pipedSpec{}

if err := json.Unmarshal(data, &ps); err != nil {

return err

}

s.ProjectID = ps.ProjectID

s.PipedID = ps.PipedID

s.PipedKeyFile = ps.PipedKeyFile

s.PipedKeyData = ps.PipedKeyData

s.Name = ps.Name

s.APIAddress = ps.APIAddress

s.WebAddress = ps.WebAddress

s.SyncInterval = ps.SyncInterval

s.AppConfigSyncInterval = ps.AppConfigSyncInterval

s.Git = ps.Git

s.Repositories = ps.Repositories

s.ChartRegistries = ps.ChartRegistries

s.ChartRepositories = ps.ChartRepositories

// Add all CloudProviders configuration as PlatformProviders configuration.

s.PlatformProviders = append(s.PlatformProviders, ps.PlatformProviders...)

s.PlatformProviders = append(s.PlatformProviders, ps.CloudProviders...)

s.AnalysisProviders = ps.AnalysisProviders

s.Notifications = ps.Notifications

s.SecretManagement = ps.SecretManagement

s.EventWatcher = ps.EventWatcher

s.AppSelector = ps.AppSelector

return nil

}

func (s *PipedSpec) UnmarshalJSON(data []byte) error {

type Alias PipedSpec

ps := &struct {

*Alias

}{

Alias: (*Alias)(s),

}

if err := json.Unmarshal(b, &ps); err != nil {

return err

}

// Add all CloudProviders configuration as PlatformProviders configuration.

s.PlatformProviders = append(s.PlatformProviders, ps.CloudProviders...)

s.CloudProviders = nil

return nil

}

Nice, thanks to teach me about that 🙏

addressed by a7a4767

…guration load as platformProviders

Co-authored-by: nghialv <nghialv2607@gmail.com>

khanhtc1202 · 2022-07-25T06:52:07Z

@nghialv @knanao it's ready to review, ptal when you guys have time 🙏

nghialv

Looks good.

knanao

let's go

khanhtc1202 requested review from nghialv and knanao as code owners July 21, 2022 06:00

github-actions bot added the area/go label Jul 21, 2022

khanhtc1202 enabled auto-merge (squash) July 21, 2022 06:10

knanao previously approved these changes Jul 21, 2022

View reviewed changes

nghialv reviewed Jul 21, 2022

View reviewed changes

pkg/app/piped/cmd/piped/piped.go Outdated Show resolved Hide resolved

khanhtc1202 dismissed knanao’s stale review via 71d3bc3 July 21, 2022 06:55

nghialv reviewed Jul 21, 2022

View reviewed changes

nghialv reviewed Jul 22, 2022

View reviewed changes

pkg/app/piped/executor/kubernetes/rollback.go Outdated Show resolved Hide resolved

nghialv reviewed Jul 22, 2022

View reviewed changes

pkg/app/server/grpcapi/piped_api.go Show resolved Hide resolved

knanao previously approved these changes Jul 24, 2022

View reviewed changes

nghialv reviewed Jul 25, 2022

View reviewed changes

khanhtc1202 disabled auto-merge July 25, 2022 04:20

nghialv reviewed Jul 25, 2022

View reviewed changes

khanhtc1202 dismissed knanao’s stale review via a7a4767 July 25, 2022 05:18

khanhtc1202 and others added 9 commits July 25, 2022 13:14

Resolve conflict

bc77b11

Remove PipedSpec.CloudProvider type and make all cloudProviders confi…

769f36d

…guration load as platformProviders

Update pkg/app/piped/cmd/piped/piped.go

c87c4b3

Co-authored-by: nghialv <nghialv2607@gmail.com>

Remove duplicate

4fd1d76

Fix error

396e039

Piped api update

7727100

Add cloudprovider

41f24ab

Update unmarshalJSON logic

247e66e

Resolve conflict

d91cf3e

khanhtc1202 force-pushed the platform-provider-logic branch from a7a4767 to d91cf3e Compare July 25, 2022 06:23

khanhtc1202 enabled auto-merge (squash) July 25, 2022 06:51

nghialv approved these changes Jul 25, 2022

View reviewed changes

knanao approved these changes Jul 25, 2022

View reviewed changes

khanhtc1202 merged commit 0fb6e72 into master Jul 25, 2022

khanhtc1202 deleted the platform-provider-logic branch July 25, 2022 09:11

github-actions bot mentioned this pull request Aug 4, 2022

Release v0.35.0 #3833

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make platformProviders works as piped deployment provider #3815

Make platformProviders works as piped deployment provider #3815

khanhtc1202 commented Jul 21, 2022

nghialv Jul 21, 2022

khanhtc1202 Jul 21, 2022

nghialv Jul 21, 2022 •

edited

nghialv Jul 21, 2022 •

edited

khanhtc1202 Jul 21, 2022

khanhtc1202 Jul 21, 2022

nghialv Jul 22, 2022

khanhtc1202 Jul 22, 2022

nghialv Jul 22, 2022

khanhtc1202 Jul 22, 2022

khanhtc1202 Jul 22, 2022

nghialv Jul 25, 2022

khanhtc1202 commented Jul 25, 2022

nghialv Jul 25, 2022

khanhtc1202 Jul 25, 2022

khanhtc1202 Jul 25, 2022

khanhtc1202 commented Jul 25, 2022

nghialv left a comment

knanao left a comment

Make platformProviders works as piped deployment provider #3815

Make platformProviders works as piped deployment provider #3815

Conversation

khanhtc1202 commented Jul 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nghialv Jul 21, 2022 • edited

Choose a reason for hiding this comment

nghialv Jul 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khanhtc1202 commented Jul 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khanhtc1202 commented Jul 25, 2022

nghialv left a comment

Choose a reason for hiding this comment

knanao left a comment

Choose a reason for hiding this comment

nghialv Jul 21, 2022 •

edited

nghialv Jul 21, 2022 •

edited