Skip to content

Implement deployment manager autogen template #5

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

Merged
merged 13 commits into from
Jul 6, 2020
Merged

Conversation

gibbleyg
Copy link
Contributor

No description provided.

gibbleyg and others added 10 commits June 22, 2020 19:47
Change-Id: Ife52798a74fb77d34b5252ce02ac85a5558703d7
Change-Id: Ibda9737fb6921784554b44fa010efc133dcc23e3
Change-Id: Ifdce705ad18fc494e743112196aa98c8a1208955
Change-Id: I9009994199ed35e6a5f987a94a47ad1d2d569c97
Change-Id: I6db0211255edbfb18f77838827acb24cda54e04e
Change-Id: I0eaa9d851d0ed674f2f69f5a46b491d6cf696f0a
Change-Id: Iad21dc494b6ede97050c13c0fb92ca4fac795196
Change-Id: Id646e7772d8c43905a032bd0b44fb76769c16366
@gibbleyg gibbleyg requested a review from huyhg June 27, 2020 00:15
@@ -29,4 +29,4 @@ RUN go build -v -o /usr/local/bin/autogen

FROM alpine:latest
COPY --from=0 /usr/local/bin/autogen /usr/local/bin/autogen
CMD ["autogen"]
ENTRYPOINT ["autogen"]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: perhaps use the full path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing

}

cmd.Flags().StringVar(&c.PartnerID, "partnerId", "", "partner id for autogen template")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need these? Aren't they already in the KRM resource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This autogen container accepts the autogen.yaml file, which doesn't contain the partnerId or solutionId, so I'm passing these as cmd line args

resources = append(resources, resource)
}

// TODO: Setup execution order for apply command
Copy link
Contributor

Choose a reason for hiding this comment

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

File an issue to track, and change this to TODO(#ISSUE_NUMBER)

Copy link
Contributor

Choose a reason for hiding this comment

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

The registry type suggested in another comment should have the functionality of determining the dependencies. Resource then will need a method to list its dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding TODO #8

@@ -20,7 +20,8 @@ import (
)

// GetMpdevCommands returns all command.
func GetMpdevCommands(name string) (c []*cobra.Command) {
func GetMpdevCommands(name string) []*cobra.Command {
var c []*cobra.Command
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it must have gotten there when I was doing some merging, removing

@@ -16,8 +16,7 @@ package apply

// PackerGceImageBuilder uses Packer to create a GCEImage when applied
type PackerGceImageBuilder struct {
TypeMeta
ObjectMeta
ResourceShared
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps BaseResource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing

}
dm.outDir = dir

fmt.Printf("Generating deployment manager template from autogen\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


gcsPath := fmt.Sprintf("gs://%s/%s", dm.GCS.Bucket, dm.GCS.Object)

cmd := exec.Command("gsutil", "cp", "-r", dmTemplate.outDir, gcsPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to zip the files. That could be a default option in the resource configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

cp := &ContainerProcess{
containerImage: autogenImg,
processArgs: args,
mounts: []string{fmt.Sprintf("type=bind,src=%s,dst=/autogen", dm.outDir)},
Copy link
Contributor

Choose a reason for hiding this comment

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

ContainerProcess should provide some typed configurations for these mounts, instead of asking for a docker specific string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will change

@gibbleyg gibbleyg requested a review from huyhg July 2, 2020 16:48
resources = append(resources, resource)
}

// TODO(https://github.com/GoogleCloudPlatform/marketplace-tools/issues/8): Setup execution order for apply command
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think #8 should suffice

@gibbleyg
Copy link
Contributor Author

gibbleyg commented Jul 6, 2020

@googlebot rescan

@gibbleyg gibbleyg merged commit 6e28c56 into master Jul 6, 2020
@gibbleyg gibbleyg deleted the gibbley/autogen branch July 6, 2020 22:27
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.

2 participants