Skip to content
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 issues with PR #5631 causing base64 errors #5492

Merged
merged 1 commit into from
Nov 15, 2018

Conversation

jkozera
Copy link

@jkozera jkozera commented Nov 15, 2018

What did you implement:

Closes #5491

How did you implement it:

Details, why the changes were required:

  • Some of values were not converted back from base64, which resulted in spurious errors with base64 strings. (This was caused by nested arrays, like commands, not being converted back, combined with the wrong assumption that commands always occur before options. It is not always true, as the added test case demonstrates.)
  • Some other values were converted from base64, but they weren't base64 in the first place, which resulted in junk binary data. (This was due to the invalid assumption that only odd-numbered values need to be converted.)

How can we verify it:

Try running for instance serverless --no-color --verbose info in any project.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

- Some of values were not converted back from base64, which resulted in
  spurious errors with base64 strings.
  (This was caused by nested arrays, like commands, not being converted
   back, combined with the wrong assumption that commands always occur
   before options. It is not always true, as the added test case
   demonstrates.)
- Some other values were converted *from* base64, but they weren't
  base64 in the first place, which resulted in junk binary data.
  (This was due to the invalid assumption that only odd-numbered values
   need to be converted.)
Copy link
Contributor

@dschep dschep left a comment

Choose a reason for hiding this comment

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

Huge thanks for the quick fix @jkozera!!!

@dschep dschep merged commit 8138f56 into serverless:master Nov 15, 2018
@Enase
Copy link
Contributor

Enase commented Nov 15, 2018

thanks +1

dschep added a commit that referenced this pull request Nov 15, 2018
@medikoo
Copy link
Contributor

medikoo commented Nov 15, 2018

When it is expected to be published?

Technically SLS v1.33 is quite broken now (unusable for my projects), if that's not meant to be patched quickly, then at least it'd be good to mark v1.33 as deprecated

@jkozera
Copy link
Author

jkozera commented Nov 15, 2018

FWIW , it can be worked around by always passing arguments like:

sls [command] [subcommand] --arg1 [value] --arg2 [value]

Basically, never placing commands after other options, and never using booleans without arguments. For the booleans, you can pass 1, for instance:

sls [command] [subcommand] --arg1 [value] --arg2 [value] --no-color 1

But yeah, it's a pain, and probably a worse workaround than just downgrading to v1.32. :)

UPDATE: Errm, nevermind, even this doesn't always work:

$ ./bin/serverless info --no-color 1 --verbose 1           
 
  Serverless Error ---------------------------------------
 
  Serverless command "info MQ==" not found. Did you mean "info"? Run "serverless help" for a list of all available commands.

@medikoo
Copy link
Contributor

medikoo commented Nov 15, 2018

For me following commands as simple as below crash

$ npx sls deploy -v -s somestage
 
  Serverless Error ---------------------------------------
 
  Serverless command "deploy somestage" not found. Did you mean "deploy"? Run "serverless help" for a list of all available commands.

Even if there's a workaround, I wouldn't expect anyone to take effort and learn about it.

It'll be best if v1.33 is just deprecated, as current behavior is pretty nasty.

@dschep
Copy link
Contributor

dschep commented Nov 15, 2018

i'm releasing 1.33.1 asap

dschep added a commit that referenced this pull request Nov 15, 2018
@dschep
Copy link
Contributor

dschep commented Nov 15, 2018

and it's released.

@medikoo
Copy link
Contributor

medikoo commented Nov 15, 2018

@dschep Thank you!

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.

4 participants