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

Revert "feat(provider/aws): Functions (listing and searching)" #7567

Merged
merged 2 commits into from
Oct 25, 2019

Conversation

plumpy
Copy link
Member

@plumpy plumpy commented Oct 25, 2019

Reverts #7536

This breaks things pretty dramatically for me. @maggieneterval reproduced the breakage.

When you click on a server group, the details pane doesn't open. In the console, I see 404 responses to this URL:
https://localhost:8084/applications/plumpstuff/functions

I don't know if this is breaking because there's some feature I don't have enabled or maybe there are unmerged changes to support this that go into gate and/or clouddriver? But it's pretty badly broken, so I'm going to roll it back.

@plumpy
Copy link
Member Author

plumpy commented Oct 25, 2019

cc @sidmuls

@spinnakerbot
Copy link
Contributor

The following commits need their title changed:

Please format your commit title into the form:

<type>(<scope>): <subject>, e.g. fix(kubernetes): address NPE in status check

This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here.

@plumpy plumpy added the ready to merge Reviewed and ready for merge label Oct 25, 2019
@erikmunson
Copy link
Member

👍 I logged on to do the same, only to see this PR. As far as I can tell there are a couple reasons:

  1. There is no feature flag for functions, and as a result the function data source always adds an item to the navigation menu under Infrastructure and starts trying to poll for data, whether you wanted that capability or not.
  2. As plumpy noted, when there are no functions in an app the /application/<app>/functions endpoint returns a 404, not an empty array like the other infrastructure endpoints (server groups, lbs, etc.). Data sources do not handle failed responses very well, and as plumpy observed one of the fun things about failed responses is that they can cascade out of a single data source and mess with other things like the ability to show the details pane for other kinds of infra.

I'm wondering if #2 is actually because (per plumpy's description) there is a flag for the functions controller in gate, and this is the behavior when that flag is off? If so, perhaps #2 is ok and expected behavior in this case. Either way, this should be feature flagged per #1, but if this is in fact he behavior of that endpoint when functions are enabled then that should change as well.

@maggieneterval
Copy link
Contributor

@erikmunson was about to add basically the exact same comment 😄
+1 to adding a feature flag and not registering the functions data source if disabled, similarly to how we handle entity tags.

@mergify mergify bot added the auto merged Merged automatically by a bot label Oct 25, 2019
@plumpy plumpy assigned plumpy and unassigned plumpy Oct 25, 2019
@mergify mergify bot merged commit e49ffaf into master Oct 25, 2019
christopherthielen added a commit to christopherthielen/deck that referenced this pull request Oct 28, 2019
d71daa5 fix(core/pipeline): fully re-render list of trigger configs after a delete (spinnaker#7571)
629a98f fix(core/pipeline): make revision dropdown usable, layout tweaks (spinnaker#7569)
ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568)
e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567)
114303a fix(kubernetes): disable project cluster filtration by stack/detail (spinnaker#7562)
86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536)
6236a9f fix(core/pipeline): make UX less bad when a pipeline stage never happened (spinnaker#7563)
christopherthielen added a commit to christopherthielen/deck that referenced this pull request Oct 28, 2019
c2bbf20 feat(rosco): Allow roscoMode per stage/execution (spinnaker#7564)
ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568)
e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567)
86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536)
christopherthielen added a commit that referenced this pull request Oct 28, 2019
d71daa5 fix(core/pipeline): fully re-render list of trigger configs after a delete (#7571)
629a98f fix(core/pipeline): make revision dropdown usable, layout tweaks (#7569)
ca176fc feat(provider/aws): Functions (listing and searching) (#7568)
e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (#7536)" (#7567)
114303a fix(kubernetes): disable project cluster filtration by stack/detail (#7562)
86a365b feat(provider/aws): Functions (listing and searching) (#7536)
6236a9f fix(core/pipeline): make UX less bad when a pipeline stage never happened (#7563)
christopherthielen added a commit that referenced this pull request Oct 28, 2019
c2bbf20 feat(rosco): Allow roscoMode per stage/execution (#7564)
ca176fc feat(provider/aws): Functions (listing and searching) (#7568)
e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (#7536)" (#7567)
86a365b feat(provider/aws): Functions (listing and searching) (#7536)
Jammy-Louie pushed a commit to pivotal/deck that referenced this pull request Nov 8, 2019
Jammy-Louie pushed a commit to pivotal/deck that referenced this pull request Nov 8, 2019
d71daa5 fix(core/pipeline): fully re-render list of trigger configs after a delete (spinnaker#7571)
629a98f fix(core/pipeline): make revision dropdown usable, layout tweaks (spinnaker#7569)
ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568)
e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567)
114303a fix(kubernetes): disable project cluster filtration by stack/detail (spinnaker#7562)
86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536)
6236a9f fix(core/pipeline): make UX less bad when a pipeline stage never happened (spinnaker#7563)
Jammy-Louie pushed a commit to pivotal/deck that referenced this pull request Nov 8, 2019
c2bbf20 feat(rosco): Allow roscoMode per stage/execution (spinnaker#7564)
ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568)
e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567)
86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536)
@christopherthielen christopherthielen deleted the revert-7536-master branch March 15, 2020 20:08
yunzhangit pushed a commit to yunzhangit/deck that referenced this pull request Mar 28, 2021
yunzhangit pushed a commit to yunzhangit/deck that referenced this pull request Mar 28, 2021
d71daa5 fix(core/pipeline): fully re-render list of trigger configs after a delete (spinnaker#7571)
629a98f fix(core/pipeline): make revision dropdown usable, layout tweaks (spinnaker#7569)
ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568)
e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567)
114303a fix(kubernetes): disable project cluster filtration by stack/detail (spinnaker#7562)
86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536)
6236a9f fix(core/pipeline): make UX less bad when a pipeline stage never happened (spinnaker#7563)
yunzhangit pushed a commit to yunzhangit/deck that referenced this pull request Mar 28, 2021
c2bbf20 feat(rosco): Allow roscoMode per stage/execution (spinnaker#7564)
ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568)
e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567)
86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Reviewed and ready for merge target-release/1.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants