Determine artifact version for Cloud Run #3368

ffjlabo · 2022-03-09T04:31:10Z

What this PR does / why we need it:

Add rich version info for Cloud Run

Which issue(s) this PR fixes:

A part of #3303

Does this PR introduce a user-facing change?:

NONE

pipecd-bot · 2022-03-09T04:32:01Z

The following files are not gofmt-ed. By commenting /golinter fmt, the formatted one will be appended to this pull request automatically.

pkg/app/piped/cloudprovider/cloudrun/servicemanifest.go

--- pkg/app/piped/cloudprovider/cloudrun/servicemanifest.go.orig
+++ pkg/app/piped/cloudprovider/cloudrun/servicemanifest.go
@@ -19,11 +19,12 @@
 	"os"
 	"strings"
 
-	"github.com/pipe-cd/pipecd/pkg/model"
 	"google.golang.org/api/run/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/yaml"
+
+	"github.com/pipe-cd/pipecd/pkg/model"
 )
 
 type ServiceManifest struct {

pkg/app/piped/cloudprovider/cloudrun/servicemanifest_test.go

--- pkg/app/piped/cloudprovider/cloudrun/servicemanifest_test.go.orig
+++ pkg/app/piped/cloudprovider/cloudrun/servicemanifest_test.go
@@ -17,9 +17,10 @@
 import (
 	"testing"
 
-	"github.com/pipe-cd/pipecd/pkg/model"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/pipe-cd/pipecd/pkg/model"
 )
 
 const serviceManifest = `

pipecd-bot · 2022-03-09T04:37:00Z

Code coverage for golang is 32.40%. This pull request increases coverage by 0.05%.

File	Function	Base	Head	Diff
pkg/app/piped/cloudprovider/cloudrun/servicemanifest.go	FindArtifactVersion	`--`	`80.00%`	`+80.00%`

khanhtc1202 · 2022-03-09T04:44:17Z

pkg/app/piped/cloudprovider/cloudrun/servicemanifest.go

 	"os"
 	"strings"

+	"github.com/pipe-cd/pipecd/pkg/model"


nits, lint error. Please refer to other files for importing conventions.

pipecd-bot · 2022-03-09T04:57:45Z

Code coverage for golang is 32.40%. This pull request increases coverage by 0.05%.

File	Function	Base	Head	Diff
pkg/app/piped/cloudprovider/cloudrun/servicemanifest.go	FindArtifactVersion	`--`	`80.00%`	`+80.00%`

knanao · 2022-03-09T04:56:28Z

pkg/app/piped/cloudprovider/cloudrun/servicemanifest_test.go

+			if tc.wantErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			require.Equal(t, tc.want, got)


nits

Suggested change

if tc.wantErr {

require.Error(t, err)

return

}

require.NoError(t, err)

require.Equal(t, tc.want, got)

assert.Equal(t, tc.wantErr, err != nil)

assert.Equal(t, tc.want, got)

knanao · 2022-03-09T05:05:59Z

pkg/app/piped/planner/cloudrun/cloudrun.go

+	if av, e := p.determineArtifactVersion(ds.AppDir, cfg.Input.ServiceManifestFile); e == nil {
+		out.Versions = []*model.ArtifactVersion{av}
+	} else {
+		in.Logger.Warn("unable to determine target versions", zap.Error(e))
+		out.Versions = []*model.ArtifactVersion{
+			{
+				Kind:    model.ArtifactVersion_UNKNOWN,
+				Version: "unknown",
+			},
+		}
+	}
+


nits

Suggested change

if av, e := p.determineArtifactVersion(ds.AppDir, cfg.Input.ServiceManifestFile); e == nil {

out.Versions = []*model.ArtifactVersion{av}

} else {

in.Logger.Warn("unable to determine target versions", zap.Error(e))

out.Versions = []*model.ArtifactVersion{

{

Kind: model.ArtifactVersion_UNKNOWN,

Version: "unknown",

},

}

}

av, err := p.determineArtifactVersion(ds.AppDir, cfg.Input.ServiceManifestFile)

if err != nil {

in.Logger.Warn("unable to determine target versions", zap.Error(e))

av = &model.ArtifactVersion{

Kind: model.ArtifactVersion_UNKNOWN,

Version: "unknown",

}

}

out.Versions = []*model.ArtifactVersion{av}

pipecd-bot · 2022-03-09T05:24:30Z

Code coverage for golang is 32.40%. This pull request increases coverage by 0.05%.

File	Function	Base	Head	Diff
pkg/app/piped/cloudprovider/cloudrun/servicemanifest.go	FindArtifactVersion	`--`	`80.00%`	`+80.00%`

ffjlabo · 2022-03-09T05:42:35Z

Build & Test on github actions failed but it seems to be experimental.

khanhtc1202 · 2022-03-09T06:07:11Z

/rebase

pipecd-bot · 2022-03-09T06:07:17Z

@khanhtc1202: Rebased this pull request in response to this comment.

nghialv · 2022-03-09T06:11:17Z

pkg/app/piped/cloudprovider/cloudrun/servicemanifest.go

 	return
 }
+
+func FindArtifactVersion(sm ServiceManifest) (*model.ArtifactVersion, error) {


To be unified let's how about returning a list of version.

Suggested change

func FindArtifactVersion(sm ServiceManifest) (*model.ArtifactVersion, error) {

func FindArtifactVersions(sm ServiceManifest) ([]model.ArtifactVersion, error) {

That sounds nice!
I did because Cloud Run can use only one image in one app.
So I'll fix to change returning a list of version and insert comment about that.

pipecd-bot · 2022-03-09T06:13:14Z

Code coverage for golang is 32.40%. This pull request increases coverage by 0.05%.

File	Function	Base	Head	Diff
pkg/app/piped/cloudprovider/cloudrun/servicemanifest.go	FindArtifactVersion	`--`	`80.00%`	`+80.00%`

…:pipe-cd/pipecd into introduce_artifact_version_for_cloud_run

pipecd-bot · 2022-03-09T07:25:44Z

Code coverage for golang is 32.40%. This pull request increases coverage by 0.05%.

File	Function	Base	Head	Diff
pkg/app/piped/cloudprovider/cloudrun/servicemanifest.go	FindArtifactVersions	`--`	`80.00%`	`+80.00%`

nghialv · 2022-03-09T07:53:54Z

pkg/app/piped/planner/cloudrun/cloudrun.go

+	versions, err := p.determineVersions(ds.AppDir, cfg.Input.ServiceManifestFile)
+	if err != nil {
+		in.Logger.Warn("unable to determine target versions", zap.Error(err))
+		versions = []*model.ArtifactVersion{
+			{
+				Kind:    model.ArtifactVersion_UNKNOWN,
+				Version: "unknown",
+			},
+		}
+	}
+	out.Versions = versions


Suggested change

versions, err := p.determineVersions(ds.AppDir, cfg.Input.ServiceManifestFile)

if err != nil {

in.Logger.Warn("unable to determine target versions", zap.Error(err))

versions = []*model.ArtifactVersion{

{

Kind: model.ArtifactVersion_UNKNOWN,

Version: "unknown",

},

}

}

out.Versions = versions

out.Versions, err = p.determineVersions(ds.AppDir, cfg.Input.ServiceManifestFile)

if err != nil {

in.Logger.Warn("unable to determine target versions", zap.Error(err))

out.Versions = []*model.ArtifactVersion{

{

Kind: model.ArtifactVersion_UNKNOWN,

Version: "unknown",

},

}

}

pipecd-bot · 2022-03-09T08:08:30Z

Code coverage for golang is 32.40%. This pull request increases coverage by 0.05%.

File	Function	Base	Head	Diff
pkg/app/piped/cloudprovider/cloudrun/servicemanifest.go	FindArtifactVersions	`--`	`80.00%`	`+80.00%`

nghialv · 2022-03-09T08:20:27Z

Great. Thank you.
/lgtm

khanhtc1202 · 2022-03-09T10:35:24Z

Nice work
/approve

pipecd-bot · 2022-03-09T10:35:27Z

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.

Determine artifact version for Cloud Run

223d08c

pipecd-bot added release-note-none size/L area/piped labels Mar 9, 2022

pipecd-bot requested review from khanhtc1202, knanao and nghialv March 9, 2022 04:31

khanhtc1202 reviewed Mar 9, 2022

View reviewed changes

Fix import order

66331d2

knanao reviewed Mar 9, 2022

View reviewed changes

Small fix

171fc04

ffjlabo added 3 commits March 9, 2022 06:07

Determine artifact version for Cloud Run

f68b629

Fix import order

5c77977

Small fix

321c0a2

pipecd-bot force-pushed the introduce_artifact_version_for_cloud_run branch from 171fc04 to 321c0a2 Compare March 9, 2022 06:07

nghialv reviewed Mar 9, 2022

View reviewed changes

ffjlabo added 2 commits March 9, 2022 16:17

Merge branch 'introduce_artifact_version_for_cloud_run' of github.com…

64c523d

…:pipe-cd/pipecd into introduce_artifact_version_for_cloud_run

Fix to returning a list of artifact versions

4f14f2c

nghialv reviewed Mar 9, 2022

View reviewed changes

Small fix

4752baa

pipecd-bot added the lgtm label Mar 9, 2022

pipecd-bot added the approved label Mar 9, 2022

pipecd-bot merged commit 21874cc into master Mar 9, 2022

pipecd-bot deleted the introduce_artifact_version_for_cloud_run branch March 9, 2022 10:43

This was referenced Mar 17, 2022

Fix bug that the specified kubeConfigPath in Piped config was not used while handling manifests #3410

Merged

Use the latest codegen #3414

Merged

Delete unneeded CODEOWNERS file #3416

Merged

	func FindArtifactVersion(sm ServiceManifest) (*model.ArtifactVersion, error) {
	func FindArtifactVersions(sm ServiceManifest) ([]model.ArtifactVersion, error) {

Determine artifact version for Cloud Run #3368

Determine artifact version for Cloud Run #3368

Uh oh!

Conversation

ffjlabo commented Mar 9, 2022

Uh oh!

pipecd-bot commented Mar 9, 2022

Uh oh!

pipecd-bot commented Mar 9, 2022

Uh oh!

khanhtc1202 Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Mar 9, 2022

Uh oh!

knanao Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

knanao Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Mar 9, 2022

Uh oh!

ffjlabo commented Mar 9, 2022

Uh oh!

khanhtc1202 commented Mar 9, 2022

Uh oh!

pipecd-bot commented Mar 9, 2022

Uh oh!

nghialv Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

ffjlabo Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Mar 9, 2022

Uh oh!

pipecd-bot commented Mar 9, 2022

Uh oh!

nghialv Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Mar 9, 2022

Uh oh!

nghialv commented Mar 9, 2022

Uh oh!

khanhtc1202 commented Mar 9, 2022

Uh oh!

pipecd-bot commented Mar 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

khanhtc1202 Mar 9, 2022 •

edited

Loading

knanao Mar 9, 2022 •

edited

Loading