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

HTTP API: Switch to default stage deployments #7331

Merged
merged 3 commits into from
Feb 17, 2020

Conversation

medikoo
Copy link
Contributor

@medikoo medikoo commented Feb 13, 2020

As @Nemo64 suggested here: #7052 (comment) There's no valid point to nest endpoint behind stage prefixes.

This PR improves that, so we rely on $default stage instead

@medikoo medikoo self-assigned this Feb 13, 2020
@medikoo medikoo changed the title HTTP API, switch to default stage deployments HTTP API: Switch to default stage deployments Feb 13, 2020
@medikoo medikoo mentioned this pull request Feb 13, 2020
7 tasks
@codecov-io
Copy link

codecov-io commented Feb 13, 2020

Codecov Report

Merging #7331 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7331      +/-   ##
==========================================
+ Coverage   87.92%   87.92%   +<.01%     
==========================================
  Files         240      240              
  Lines        8897     8898       +1     
==========================================
+ Hits         7823     7824       +1     
  Misses       1074     1074
Impacted Files Coverage Δ
...lugins/aws/package/compile/events/httpApi/index.js 84.81% <100%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3910df1...941a1d1. Read the comment docs.

@Nemo64
Copy link

Nemo64 commented Feb 13, 2020

Awesome 👍

I got to admit that i'm not too deep into the httpApi but:
Why do you add the stage when the routing does not contain '*'? The routing references the HttpApi, not the Stage and in my 1 test (that admittedly didn't contain routing), the $default stage was created automatically. Is that not the case when routing is used?

pmuens
pmuens previously approved these changes Feb 13, 2020
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.

The question which immediately came to my mind was whether this would result in a breaking change.

It seems like it would but we'll release it regardless (#7052 (comment)). Because of that I'm fine with merging it (although I strongly disagree with releasing it as a minor release).

LGTM

@medikoo
Copy link
Contributor Author

medikoo commented Feb 13, 2020

The question which immediately came to my mind was whether this would result in a breaking change.

Yes, it's definitely a breaking change. I talked about with @ganeshvlrk and we decide it to push it now, as functionality was not that advertised yet (we may also treat it as "design bug fix")

Also it appears that community wants it that way. At least nobody objected, and we have just +1's on that update.

If v2 was around the corner, I would vote to push it with that, but it appears it still isn't

@medikoo
Copy link
Contributor Author

medikoo commented Feb 13, 2020

Why do you add the stage when the routing does not contain '*'?

@Nemo64 to have it really deployed, it needs some stage. While AWS automatically creates a default auto deployed stage only when we configure $default route on API level

Framework for each stage creates new API,
therefore there's no point in generating a stage
@medikoo medikoo force-pushed the 0212-http-api-no-stage-deployments branch from 245c4a4 to 941a1d1 Compare February 17, 2020 00:17
@medikoo medikoo merged commit 29b37d7 into master Feb 17, 2020
@medikoo medikoo deleted the 0212-http-api-no-stage-deployments branch February 17, 2020 00:41
@tomchiverton
Copy link

We make good use of the default being to prefix all routes with "dev", and then when all the tests pass we run an additional sls deploy giving --stage live to update the live environments.

It sounds like this default behaviour is going away in a minor release (which ?) ?

If so, is it a case of sticking something like ${self.stage,'dev'} at the front of all the resource URLs to recover our existing deployment process ?

@medikoo
Copy link
Contributor Author

medikoo commented Feb 20, 2020

@tomchiverton the way Serverless Framework deploys HTTP API, is that for each stage different API is created (so main url is different).

In that case adding stage prefix was superfluous, and that was removed with this PR.
It was already published with v1.64.0

@tomchiverton
Copy link

We have published APIs we will need to update, was I right about getting our old style URLs (i.e. the host and path is the same after we update using v1.64) it using dynamic route names ?

@camhart
Copy link

camhart commented Feb 20, 2020

You can't just push breaking changes like this. Especially if it's not clearly outlined as a breaking change anywhere in the release notes. Only reason I caught it was because I read carefully enough to be concerned.

Big down vote on this.

Also, for everyone who already has deployed using older versions of the framework, how do we work around this breaking change going forward?

@medikoo
Copy link
Contributor Author

medikoo commented Feb 20, 2020

@camhart this was proposed by community (#7052 (comment)) and we were discussing it openly whether it's right to issue such update now (#7052 (comment)). There were just upvotes, and no concerns raised.

We also considered it as design bug, that it's fine to fix in this early stage (before we really communicated support for HTTP API)

@camhart
Copy link

camhart commented Feb 20, 2020

@medikoo How many people use the serverless framework already? And how many do you think saw #7052 (comment)?

The time between a random github comment being created, and the feature breaking change being published was 10 days. There is a single upvote. I hardly count that as satisfactory.

This is going to break every ones deployments in a significant manner. This needed to wait until V2.

I'd also like to ask, has anyone researched and documented workarounds for those impacted by the change?

@medikoo
Copy link
Contributor Author

medikoo commented Feb 20, 2020

@camhart I'm sorry it caused issue for you. I was very reluctant to follow that, but again we were just advised to do that from many sides (there were 5+ different voices saying let's do that, and no single objection)

Once we release v2, we should be able to release major updates more frequently, then we will also be more cautious with stuff that eventually may break

@camhart
Copy link

camhart commented Feb 20, 2020

@medikoo There weren't rejections because the only people paying attention to the issue were people wanting the beneficial outcome from it. People who'd be impacted by it had no idea they should be looking for that ticket.

I honestly think this needs to be rolled back. 5 voices wanting it, yet serverless is likely used by tens of thousands (just a guess on numbers there)?

Someone absolutely needs to update the Release notes to outline the breaking change.

@medikoo
Copy link
Contributor Author

medikoo commented Feb 20, 2020

@camhart can you raise this issue at #7052, and ask others to vote on that?

You may also announce it on Twitter and other channels.

If we'd see there's significant wish to have it rolled back we will follow

(so far we have 1 vote for that I take, right?)

@camhart
Copy link

camhart commented Feb 20, 2020

I've just posted #7052 (comment).
@medikoo I have < 100 twitter followers--none of which likely use serverless. The release was published 2 days ago. Give it time, more will come.

@tomchiverton
Copy link

Geez. You didn't even post the breaking change to your own forums. Bad.

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

Successfully merging this pull request may close these issues.

None yet

6 participants