logging upcoming breaking changes by eahefnawy · Pull Request #3217 · serverless/serverless

eahefnawy · 2017-02-09T10:09:22Z

What did you implement:

Added functionality for listing all the upcoming breaking changes in the CLI. This is going to be one of each release steps.

How did you implement it:

Added a method to the CLI that logs all the breaking changes. User can opt-out by setting the SLS_IGNORE_WARNING env var. I've also updated the release checklist to include a step where we update the upcoming breaking changes list on every release.

How can we verify it:

open the CLI class file and add some strings to the breakingChanges array.
run any serverless command and you should see the list.
opt out by setting the SLS_IGNORE_WARNING env var

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

pmuens

Looks good! Just pushed some minor fixes so that the list of messages are indented.

Just added one comment about a test for this method.

pmuens · 2017-02-09T10:25:47Z

    console.log(message); // eslint-disable-line no-console
  }
+
+  logBreakingChanges() {


This should have a basic test (something like that here might be sufficient:

serverless/lib/plugins/aws/info/display.test.js

Lines 43 to 60 in d98dfa3

it('should display general service info', () => {

let expectedMessage = '';

expectedMessage += `${chalk.yellow.underline('Service Information')}\n`;

expectedMessage += `${chalk.yellow('service:')} my-first\n`;

expectedMessage += `${chalk.yellow('stage:')} dev\n`;

expectedMessage += `${chalk.yellow('region:')} eu-west-1`;

expectedMessage += `\n${chalk.yellow('api keys:')}`;

expectedMessage += '\n None';

expectedMessage += `\n${chalk.yellow('endpoints:')}`;

expectedMessage += '\n None';

expectedMessage += `\n${chalk.yellow('functions:')}`;

expectedMessage += '\n None';

const message = awsInfo.display();

expect(consoleLogStub.calledOnce).to.equal(true);

expect(message).to.equal(expectedMessage);

});

).

eahefnawy · 2017-02-09T12:20:08Z

Thanks @pmuens 🙌 ... I've definitely thought about that too, but it got too challenging for it to be worth it. The main challenge is that by default there are no breaking changes and so the CLI doesn't log anything. The only way to change that is in the code (not env var, not serverless.yml..etc). So adding that test would require changing the actual logic. Furthermore, the output is different on every release depending on the breaking changes. It's a manual step that we have to do to make sure it's accurate, which is why I added it to the release checklist.

Generally speaking, covering tests for CLI outputs is not that worthwhile and too hard to maintain, otherwise, we'd have to add tests for every error message we display too.

pmuens · 2017-02-09T13:41:25Z

@eahefnawy yes, I agree. Testing the CLI is hard and not always worth the effort.

I've pushed some convenience changes (so that you don't have to scroll to the end of the CLI file to add the breaking changes). Furthermore I've restructured the code a little bit and added tests so that all the different cases are captured by unit tests.

Feel free to merge if it's GTM from your side!

eahefnawy · 2017-02-10T13:38:06Z

+      expectedMessage += chalk.yellow('  - x is broken\n');
+      expectedMessage += chalk.yellow('  - y will be updated\n');
+      expectedMessage += '\n';
+      expectedMessage += chalk.yellow('  You can opt-out from these warnings by setting the "SLS_IGNORE_WARNING=*" environment variable.\n'); //eslint-disable-line


@pmuens Really appreciate your dedication to test coverage and high quality code 🥇 , but I'm still worried about maintaining those kinda tests, especially the "expected messages" above. If we for some reason change an irrelevant character, those tests would fail and we'll need to spend considerable amount of time looking very closely at the message diff, while at the very end it doesn't really matter much. Doing that for every message we log would be hell, especially error messages.

But let's merge this anyway. It doesn't deserve anymore time.

Thanks @pmuens 🙌

Yes, I agree. checking everything in detail might be too detailed 👍 🙌

Maybe for future situations like this we can check that the error / warning message contains the Breaking change items etc. (and not the full message) so that we don't have to go through hell when we change the message itself 👹

hehe I like that! 💯

logging up coming breaking changes

2069ac5

eahefnawy added this to the 1.7 milestone Feb 9, 2017

eahefnawy self-assigned this Feb 9, 2017

added opt-out message

4ea752e

eahefnawy changed the title ~~logging up coming breaking changes~~ logging upcoming breaking changes Feb 9, 2017

Add minor aesthetic improvements

69d8726

pmuens suggested changes Feb 9, 2017

View reviewed changes

Refactor for more convenience and add tests

7044d56

pmuens approved these changes Feb 9, 2017

View reviewed changes

eahefnawy commented Feb 10, 2017

View reviewed changes

pmuens merged commit 0a818af into master Feb 10, 2017

pmuens deleted the breaking-changes-warning branch February 10, 2017 14:31

pmuens mentioned this pull request Mar 29, 2017

Remove breaking change logging #3418

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

logging upcoming breaking changes#3217

logging upcoming breaking changes#3217
pmuens merged 4 commits into
masterfrom
breaking-changes-warning

eahefnawy commented Feb 9, 2017

Uh oh!

pmuens left a comment

Uh oh!

pmuens Feb 9, 2017 •

edited

Loading

Uh oh!

eahefnawy commented Feb 9, 2017

Uh oh!

pmuens commented Feb 9, 2017

Uh oh!

eahefnawy Feb 10, 2017

Uh oh!

pmuens Feb 10, 2017 •

edited

Loading

Uh oh!

eahefnawy Feb 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	it('should display general service info', () => {
	let expectedMessage = '';

	expectedMessage += `${chalk.yellow.underline('Service Information')}\n`;
	expectedMessage += `${chalk.yellow('service:')} my-first\n`;
	expectedMessage += `${chalk.yellow('stage:')} dev\n`;
	expectedMessage += `${chalk.yellow('region:')} eu-west-1`;
	expectedMessage += `\n${chalk.yellow('api keys:')}`;
	expectedMessage += '\n None';
	expectedMessage += `\n${chalk.yellow('endpoints:')}`;
	expectedMessage += '\n None';
	expectedMessage += `\n${chalk.yellow('functions:')}`;
	expectedMessage += '\n None';

	const message = awsInfo.display();
	expect(consoleLogStub.calledOnce).to.equal(true);
	expect(message).to.equal(expectedMessage);
	});

Uh oh!

Conversation

eahefnawy commented Feb 9, 2017

What did you implement:

How did you implement it:

How can we verify it:

Todos:

Uh oh!

pmuens left a comment

Choose a reason for hiding this comment

Uh oh!

pmuens Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eahefnawy commented Feb 9, 2017

Uh oh!

pmuens commented Feb 9, 2017

Uh oh!

eahefnawy Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

pmuens Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eahefnawy Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pmuens Feb 9, 2017 •

edited

Loading

pmuens Feb 10, 2017 •

edited

Loading