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 #5664 - Rollback fails due to a timestamp parsing error #5710

Merged
merged 3 commits into from Jan 28, 2019

Conversation

Projects
4 participants
@luanmuniz
Copy link
Contributor

luanmuniz commented Jan 16, 2019

When running sls rollback -t 1546961025152 the timestamp was being parsed as a string inside a Date Object.
This would return an Invalid Date, therefore, failing to convert the timestamp into a valid date.

In here we are checking to see if the date is already a valid one, otherwise convert the timestamp to a number and try again.
We are checking first because most cases we don't want to convert to numbers.

Fix #5664

Ps: We could add another check, if the date is still invalid, throw an Error, but that's not the scope of the issue.

What did you implement:

Closes #5664

How did you implement it:

When running sls rollback -t 1546961025152 the timestamp was being parsed as a string inside a Date Object.
This would return an Invalid Date, therefore, failing to convert the timestamp into a valid date.

In here we are checking to see if the date is already a valid one, otherwise convert the timestamp to a number and try again.
We are checking first because most cases we don't want to convert to numbers.

How can we verify it:

Check #5664

Ps: Can someone guide me on how to write the tests for this? I couldn't figure out for myself.

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

Fix #5664 - Rollback fails due to a timestamp parsing error
When running sls rollback -t 1546961025152 the timestamp was being parsed as a string inside a Date Object.
This would return an Invalid Date, therefore, failing to convert the timestamp into a valid date.

In here we are checking to see if the date is already a valid one, otherwise convert the timestamp to a number and try again.
We are checking first because most cases we don't want to convert to numbers.

Fix #5664

Ps: We could add another check, if the date is still invalid, throw an Error, but that's not the scope of the issue.

Signed-off-by: Luan <luan@luanmuniz.com.br>

@eahefnawy eahefnawy self-assigned this Jan 17, 2019

@eahefnawy eahefnawy added this to In progress in Serverless via automation Jan 17, 2019

@eahefnawy eahefnawy moved this from In progress to Needs review in Serverless Jan 17, 2019

@eahefnawy
Copy link
Member

eahefnawy left a comment

Thanks @luanmuniz ... could you also add a test for this edge case? 😊 ... just wanna make sure we don't miss it in future updates. It seems that coveralls also agrees.

@eahefnawy eahefnawy moved this from Needs review to In progress in Serverless Jan 17, 2019

luanmuniz added some commits Jan 19, 2019

Create tests for #5664
Signed-off-by: Luan <luan@luanmuniz.com.br>
Fix eslint issues
Signed-off-by: Luan <luan@luanmuniz.com.br>
@luanmuniz

This comment has been minimized.

Copy link
Contributor Author

luanmuniz commented Jan 19, 2019

@eahefnawy Done! Thanks! :D

Serverless automation moved this from In progress to Reviewer approved Jan 28, 2019

@eahefnawy
Copy link
Member

eahefnawy left a comment

Thanks @luanmuniz ... LG2M!

@eahefnawy eahefnawy added this to the 1.36.4 milestone Jan 28, 2019

@eahefnawy eahefnawy merged commit 4fb3d55 into serverless:master Jan 28, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+2.2%) to 93.012%
Details

Serverless automation moved this from Reviewer approved to Done Jan 28, 2019

@shortjared shortjared added the bug label Feb 6, 2019

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