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

Fix GAE deployment version error in gcloud sdk v138.0.0. #549

Merged
merged 2 commits into from Dec 15, 2016

Conversation

Projects
None yet
2 participants
@M157q
Copy link
Contributor

M157q commented Dec 15, 2016

In gcloud sdk v138.0.0,
gcloud app deploy with args --version "" will get error below:

ERROR: (gcloud.app.deploy) argument --version/-v: Bad value []: May only
contain lowercase letters, digits, and hyphens. Must begin and end with
a letter or digit. Must not exceed 63 characters.

So, if user didn't set the version and we want to use the gcloud
default generated version in this deployment, we just don't add
--version.

Fix GAE deployment version error in gcloud sdk v138.0.0.
In gcloud sdk v138.0.0,
gcloud app deploy with args `--version ""` will get error below:

ERROR: (gcloud.app.deploy) argument --version/-v: Bad value []: May only
contain lowercase letters, digits, and hyphens. Must begin and end with
a letter or digit. Must not exceed 63 characters.

So, if user didn't set the version and we want to use the gcloud
default generated version in this deployment, we just don't need to add
`--version`.
@M157q

This comment has been minimized.

Copy link
Contributor Author

M157q commented Dec 15, 2016

Got error when used --version=""
20161215_20 55 49


No error after removed --version=""
20161215_20 56 41

@M157q

This comment has been minimized.

Modify the test case of gae for #549
If user didn't speicify the version,
we should use empty string instead of `--version=""`
@@ -72,7 +72,7 @@ def push_app
command << " --verbosity \"#{verbosity}\""
command << " --project \"#{project}\""
command << " app deploy \"#{config}\""
command << " --version \"#{version}\""
command << (version ? " --version \"#{version}\"" : '')

This comment has been minimized.

@BanzaiMan

BanzaiMan Dec 15, 2016

Member

I don't think this works if version was explicitly given as "", as unlikely as it might be. (This actually is relevant to the no_stop_previous_version option, now that I think about it.)

A more robust way for this is:

command << " --version \"#{version}\"" unless version.empty?

(And similar for no_stop_previous_version.)

This comment has been minimized.

@BanzaiMan

BanzaiMan Dec 15, 2016

Member

I'll fix this after the merge.

This comment has been minimized.

@M157q

M157q Dec 15, 2016

Author Contributor

Yes, you are right. I don't think this would work when the version is explicitly given as "" either.
The robust way you described above is better.
Same for the no_stop_previous_version.

@BanzaiMan BanzaiMan merged commit 227b2bd into travis-ci:master Dec 15, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

BanzaiMan added a commit that referenced this pull request Dec 15, 2016

M157q added a commit to M157q/dpl that referenced this pull request Dec 15, 2016

@M157q

This comment has been minimized.

Copy link
Contributor Author

M157q commented Dec 15, 2016

@BanzaiMan I got this error after using your patch.
20161216_00 09 05


Also, Travis build of this commit 30a3440 failed
https://travis-ci.org/travis-ci/dpl/builds/184266319

@BanzaiMan

This comment has been minimized.

Copy link
Member

BanzaiMan commented Dec 15, 2016

Sorry about that. That's fixed already.

@M157q

This comment has been minimized.

Copy link
Contributor Author

M157q commented Dec 15, 2016

Change back to

      def version
        options[:version] || ''
      end

and

      def no_stop_previous_version
        options[:no_stop_previous_version] || ''
      end

should fix this error, I guess?

@BanzaiMan

This comment has been minimized.

Copy link
Member

BanzaiMan commented Dec 15, 2016

The release 1.8.26 should work. See 719220e.

@M157q

This comment has been minimized.

Copy link
Contributor Author

M157q commented Dec 15, 2016

Oh, cool.
I didn't see that commit.
Actually, I am not familiar with Ruby.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment