30x / shipyardctl

command line interface for Shipyard API
Apache License 2.0
0 stars 3 forks source link

change image build env-var flag #50

Closed noahdietz closed 7 years ago

noahdietz commented 7 years ago

Fixes #40

There was confusion around the -e --env flag in shipyardctl create image being for naming an environment to target, when it is actually for specifying build-time environment variables.

whitlockjc commented 7 years ago

I thought this was a no-op change but you're changing the long hand version of the option. I thought --env was what we wanted to do to be inline with apigeetool and kubectl, and that we'd just be updating the documentation to make sure it was clear that this was not your Apigee environment name.

noahdietz commented 7 years ago

The documentation has always said that it was an environment variable, but it didn't specifically say that it wasn't an Edge env. So I figured that if explicitly saying this was for env vars wasn't enough, the flag should be changed.

noahdietz commented 7 years ago

Happy to make the docs even more explicit and not change the long hand flag though 😄

whitlockjc commented 7 years ago

I don't think changing the option name is required and it breaks consistency with all CLI tools we're trying to be similar to. The last comments I saw on #40 sounded like we'd be making changes to the documentation only.

noahdietz commented 7 years ago

Okey dokey! Yeah i was being overzealous.

The docs say Environment variable to set in the built image "KEY=VAL"

Should it include something like Environment variable to set in the built image "KEY=VAL". This is not an Apigee environment"?

whitlockjc commented 7 years ago

What sucks is that as I'm creating the issues for the upcoming refactoring, I can see the conflict. While --env makes sense for this command, it's confusing knowing that other Shipyard APIs use --env for the Apigee environment name. I'm on the fence now...

whitlockjc commented 7 years ago

The more I think about this, the more I think that your proposal makes sense. While --env is used by docker and kubectl, we're building a CLI tool for Apigee customers and apigeetool uses --environments, -e for Apigee environments.

This being said, maybe we should use --env-var for the long version? Reason being is we don't use camel casing elsewhere, and neither do the other CLIs from what I can tell. As for the short version, I'm not sure what it should be because -e and -v (the logical suggestions) are being used already.

noahdietz commented 7 years ago

Ok so we will use --env-var as the long form for environment variable specification.

Lets shelf the support of any short form here until we can come up with a better answer as the logical suggestions are taken.

whitlockjc commented 7 years ago

LGTM