Skip to content
This repository has been archived by the owner on Mar 24, 2023. It is now read-only.

Allow ship init to be pointed to valid go-getter ship.yaml path #682

Merged
merged 4 commits into from Nov 6, 2018

Conversation

Rob0h
Copy link
Contributor

@Rob0h Rob0h commented Nov 6, 2018

What I Did

  • Allow ship init to be pointed to valid go-getter ship.yaml path

How I Did it

  • Point ship init to a ship.yaml

How to verify it

Point ship init to valid go-getter ship.yaml path

Description for the Changelog

Allow ship init to be pointed to valid go-getter ship.yaml path

Picture of a Boat (not required but encouraged)

馃浂

"v1": {
"config": null,
"releaseName": "ship",
"upstream": "file::/Users/robert/go/src/github.com/replicatedhq/ship/integration/init/local-single-file-gogetter/input/ship.yaml",
Copy link
Member

Choose a reason for hiding this comment

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

Not OK

Maybe change this to <UPSTREAM> and include the upstream as part of the string replacement? (the map[string]string{} on line 94 of integration/init/integration_test.go)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yea this is why i didn't push this

Copy link
Member

Choose a reason for hiding this comment

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

We do variable substitution for all the init_app tests: https://github.com/replicatedhq/ship/blob/master/integration/init_app/amazon-eks-template/expected/.ship/state.json
Seems like the idiom is __upstream__ not <UPSTREAM> though


if isReplicatedApp {
return "inline.replicated.app", finalPath, nil
if isReplicatedApp {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this up because it checks for 2 different files, if the first is true and the second is false... 馃槩

Copy link
Member

@laverya laverya left a comment

Choose a reason for hiding this comment

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

lgtm, but we could reduce the magic there a little

Expect(err).NotTo(HaveOccurred())
absolutePath := filepath.Join(pwdRoot, "..")
absoluteUpstream = fmt.Sprintf("file::%s", filepath.Join(absolutePath, relativePath))
replacements["__upstream__"] = absolutePath
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer

Suggested change
replacements["__upstream__"] = absolutePath
replacements["__upstream__"] = absoluteUpstream

"v1": {
"config": null,
"releaseName": "ship",
"upstream": "file::__upstream__/input/ship.yaml",
Copy link
Member

Choose a reason for hiding this comment

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

and

Suggested change
"upstream": "file::__upstream__/input/ship.yaml",
"upstream": "file::__upstream__",

@Rob0h Rob0h merged commit 5067615 into replicatedhq:master Nov 6, 2018
@Rob0h Rob0h deleted the fix/init-file branch November 6, 2018 23:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants