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

Allow service to be specified as object in serverless.yml #3521

Merged
merged 2 commits into from
Apr 27, 2017

Conversation

HyperBrain
Copy link
Member

@HyperBrain HyperBrain commented Apr 26, 2017

What did you implement:

Closes #3513

How did you implement it:

If the parser encounters an object typed service property it sets service.service to the service name (retrieved from service.name). This is done for compatibility reasons because lots of plugins (core and 3rd party) use service.service directly to get the service name.
The object is stored into the service as service.serviceObject and all of its properties can be accessed with service.serviceObject.XXXXX.

In case the service property is a string, service.service is set to this and service.serviceObject is { name: XXXX }.

Additionally I added a service.getServiceName() method to provide an option for plugins to become more robust against changes in the serverless object structure and service.getServiceObject() that returns the object or objectified service definition.

How can we verify it:

Declare your service as object in the serverless.yml:

service:
  name: my-service
  foo: 1
  bar: doesnt-matter

Compatibility checks can be made with any existing serverless.yml that sets the property as string.

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 pmuens self-requested a review April 26, 2017 13:53
@pmuens pmuens added this to the 1.13 milestone Apr 26, 2017
@HyperBrain
Copy link
Member Author

I did not want to trigger another endless naming discussion 😈 . So I just named the service object property serviceObject because it describes what it is... threw away the idea of serviceAttributes because that would imply that there is an attributes declaration somewhere.

@HyperBrain HyperBrain changed the title Allow service to be specified as object in serverless.yml [WIP] Allow service to be specified as object in serverless.yml Apr 26, 2017
@pmuens pmuens mentioned this pull request Apr 27, 2017
7 tasks
@nicka
Copy link
Member

nicka commented Apr 27, 2017

Ok, this is making me really happy 💃

I have a question, we're planning to migrate an sls v0.x stack to v1.x. BUT before we can do this we should be able to append an -r to the stack name (yeah I know, just create a new stack, the problem is that the stack contains an S3 bucket with 30TB).

Looking at the PR would the following be true?

service:
  name: foo-r
getStackName();
// "foo-r"

@nicka
Copy link
Member

nicka commented Apr 27, 2017

Ahh damn that would not be enough, the final stack name would become foo-r-dev 😢 . We need to be able to calculate foo-dev-r.

@pmuens
Copy link
Contributor

pmuens commented Apr 27, 2017

Looking at the PR would the following be true?

@nicka yes, that's currently the plan.

Ahh damn that would not be enough, the final stack name would become foo-r-dev 😢 .

Hmm. That's bad. Maybe some plugin magic comes in handy here?

@nicka
Copy link
Member

nicka commented Apr 27, 2017

Once this is merged could we do?

service:
  name: foo
  stackName: ${self:service.name}-${opt:stage}-r

# OR

provider:
  stackName: ${self:service.name}-${opt:stage}-r
createStack() {
    const stackName = this.provider.naming.getStackName(); // Update this logic

@nicka
Copy link
Member

nicka commented Apr 27, 2017

Hmm. That's bad. Maybe some plugin magic comes in handy here?

Already tried this, but it's super embedded throughout the system.

We're currently running a forked version which does the following, ideally this get's refactored and uses a general way to overwrite stack names.

// Stack
  getStackName() {
    const name = `${this.provider.serverless.service.service}-${this.provider.getStage()}`;

    if (this.provider.serverless.service.provider.preV1Resources) {
      return `${name}-r`;
    }

    return name;
  },

@HyperBrain
Copy link
Member Author

HyperBrain commented Apr 27, 2017

@nicka Agree that setting a custom stack name is a very valuable feature. But I think it is out of scope for this PR as it handles the possibility to specify the service as object.

I suggest opening an additional feature request "Allow setting a custom stackname" that can be implemented after this has been merged. It could specify that service.stackName can be used to set the value. Additionally the logic of getStackName() and any other hard-coded service name references should be replaced in the context of this new feature request.

/cc @eahefnawy @pmuens

Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

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

Nice! Just reviewed it @HyperBrain

Exactly what we need. Also big +1 for taking care of backwards compatibility 🥇

GTM :shipit:

@pmuens pmuens merged commit d88dced into serverless:master Apr 27, 2017
@pmuens pmuens deleted the allow-service-as-object branch April 27, 2017 10:49
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.

None yet

3 participants