Remove requirement for an existing AWS profile on sls package command #6564

drexler · Aug 20, 2019

What did you implement:

How did you implement it:

#6427 indirectly enforced a profile check on the sls package command. This removes the check since a profile not is needed by that command

How can we verify it:

Todos:

Note: Run npm run test-ci to run all validation checks on proposed changes

Write tests and confirm existing functionality is not broken.
Validate via npm test
Write documentation
Ensure there are no lint errors.
Validate via npm run lint-updated
Note: Some reported issues can be automatically fixed by running npm run lint:fix
Ensure introduced changes match Prettier formatting.
Validate via npm run prettier-check-updated
Note: All reported issues can be automatically fixed by running npm run prettify-updated
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

medikoo

@drexler Great thanks for that PR.

Still that issue was supposedly fixed with #6447 (published with v1.49.0)

Are you sure that AWS profile validation still happens on package command?

/cc @dschep

drexler · Aug 21, 2019

@medikoo #6447 addresses a problem slightly different from this. Here, if the given serverless.yml file contains profile property, the package command will attempt to validate that. This PR is to make it unnecessary for the command - whether the profile exists or not.

serverless.yml:

provider:
  name: aws
  runtime: nodejs10.x
  profile: test_profile

medikoo

@drexler great thanks for PR!

Still implementation as proposed removes validation step in its entirety, and we don't want to do that.

As I looked into code, the problem lies in this logic:

serverless/lib/plugins/aws/lib/validate.js

Lines 18 to 30 in babc140

    
           const creds = Object.assign({}, this.provider.getCredentials()); 
        
           delete creds.region; 
        
           delete creds.signatureVersion; 
        
           if ( 
        
             Object.keys(creds).length === 0 && 
        
             !process.env.ECS_CONTAINER_METADATA_FILE && 
        
             !process.env.AWS_CONTAINER_CREDENTIALS_RELATIVE_URI && 
        
             !process.env.AWS_CONTAINER_CREDENTIALS_FULL_URI && 
        
             ['logs', 'info', 'deploy', 'remove', 'rollback', 'metrics', 'invoke'].includes( 
        
               this.serverless.processedInput.commands[0] 
        
             ) && 
        
             this.serverless.processedInput.commands[1] !== 'local' 
        
           ) {

Where first we check whether some AWS credentials are set (and eventually validate specified profile) and aftewards that we check does this validation step should occur for given command at all.

Real fix would be to check if command applies (and aborting eventually), before trying to retrieve AWS creds (with this.provider.getCredentials() involved)

Anyway I've also posted question here: #6536 (comment)
As I'd like to understand well, why we want to support keeping and invalid provider.profile setting in serverless.yml

drexler · Aug 28, 2019

@medikoo thought of that approach before i reached for the chainsaw solution... i'll update this PR

drexler · Sep 5, 2019

@medikoo updated the PR per your feedback

medikoo

@drexler great that for update! Still it's not exactly best approach

Current situation is that we have folllowing logic(in simplified form):

creds = resolveEnvCreds()
if (hasCreds(creds) & isAppicableCommand()) ensureAwsCreds();

And resolveEnvCreds() crashes for us (while isAppicableCommand() returns false). So valid solution is to improve logic seems as follows:

if (!isAppicableCommand()) return;
creds = resolveEnvCreds()
if (hasCreds(creds)) ensureAwsCreds();

While what you propsed is:

if (!isAppicableCommandPartially()) return;
creds = resolveEnvCreds()
if (hasCreds(creds) & isAppicableCommand()) ensureAwsCreds();

And that's not very clean (and prone to maintenance issues)

isAppicableCommand() is

serverless/lib/plugins/aws/lib/validate.js

Lines 26 to 29 in c4b7726

    
           ['logs', 'info', 'deploy', 'remove', 'rollback', 'metrics', 'invoke'].includes( 
        
             this.serverless.processedInput.commands[0] 
        
           ) && 
        
           this.serverless.processedInput.commands[1] !== 'local'

pmuens

Hey @drexler thanks for working on that problem!

@medikoo is already deep in the trenches with this PR so I think that he's the best to do the final code review.

I just wanted to add that we should add a regression test once the implementation looks good.

medikoo

@drexler thank you, looks great! Just one minor remark

lib/plugins/aws/lib/validate.js

medikoo

Thank you @drexler !

We're taking this in

medikoo reviewed Aug 20, 2019

View reviewed changes

medikoo added the needs feedback label Aug 20, 2019

medikoo assigned drexler Aug 20, 2019

medikoo suggested changes Aug 21, 2019

View reviewed changes

medikoo mentioned this pull request Aug 22, 2019

sls package requires AWS profile to exist #6536

Closed

medikoo added bug cat/packaging and removed needs feedback labels Aug 22, 2019

medikoo added this to the 1.51.0 milestone Aug 22, 2019

pmuens modified the milestones: 1.51.0, 1.52.0 Aug 27, 2019

drexler force-pushed the credentials-check branch 2 times, most recently from 6359a4b to fbbbb2b Compare September 4, 2019 12:22

medikoo suggested changes Sep 5, 2019

View reviewed changes

drexler force-pushed the credentials-check branch from fbbbb2b to 6157177 Compare September 7, 2019 01:22

pmuens requested a review from medikoo September 9, 2019 09:15

pmuens added the pr/in-progress label Sep 9, 2019

pmuens reviewed Sep 9, 2019

View reviewed changes

medikoo reviewed Sep 9, 2019

View reviewed changes

lib/plugins/aws/lib/validate.js Outdated Show resolved Hide resolved

pmuens modified the milestones: 1.52.0, 1.53.0 Sep 10, 2019

drexler added 2 commits September 11, 2019 07:49

fix: remove credential validation check from sls package command

381c147

test: update covering tests

5e425a7

drexler force-pushed the credentials-check branch from 6157177 to 5e425a7 Compare September 11, 2019 11:53

medikoo approved these changes Sep 11, 2019

View reviewed changes

medikoo merged commit d2e75e1 into serverless:master Sep 11, 2019

medikoo removed the pr/in-progress label Sep 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove requirement for an existing AWS profile on sls package command #6564

Remove requirement for an existing AWS profile on sls package command #6564

Uh oh!

drexler commented Aug 20, 2019

Uh oh!

medikoo left a comment

Uh oh!

drexler commented Aug 21, 2019

Uh oh!

medikoo left a comment

Uh oh!

drexler commented Aug 28, 2019

Uh oh!

drexler commented Sep 5, 2019

Uh oh!

medikoo left a comment •

edited

Loading

Uh oh!

pmuens left a comment

Uh oh!

medikoo left a comment

Uh oh!

Uh oh!

medikoo left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	const creds = Object.assign({}, this.provider.getCredentials());
	delete creds.region;
	delete creds.signatureVersion;
	if (
	Object.keys(creds).length === 0 &&
	!process.env.ECS_CONTAINER_METADATA_FILE &&
	!process.env.AWS_CONTAINER_CREDENTIALS_RELATIVE_URI &&
	!process.env.AWS_CONTAINER_CREDENTIALS_FULL_URI &&
	['logs', 'info', 'deploy', 'remove', 'rollback', 'metrics', 'invoke'].includes(
	this.serverless.processedInput.commands[0]
	) &&
	this.serverless.processedInput.commands[1] !== 'local'
	) {

Search code, repositories, users, issues, pull requests...

Remove requirement for an existing AWS profile on sls package command #6564

Remove requirement for an existing AWS profile on sls package command #6564

Uh oh!

Conversation

drexler commented Aug 20, 2019

What did you implement:

How did you implement it:

How can we verify it:

Todos:

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

drexler commented Aug 21, 2019

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

drexler commented Aug 28, 2019

Uh oh!

drexler commented Sep 5, 2019

Uh oh!

medikoo left a comment • 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!

pmuens left a comment

Choose a reason for hiding this comment

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

medikoo left a comment •

edited

Loading