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

Correct api.getFluxImages usage #3233

Merged
merged 1 commit into from
Jun 20, 2018
Merged

Correct api.getFluxImages usage #3233

merged 1 commit into from
Jun 20, 2018

Conversation

aaron7
Copy link
Contributor

@aaron7 aaron7 commented Jun 20, 2018

api.getFluxImages takes 3 parameters: getFluxImages(id, serviceId = null, containerFields = []) with a recent change adding the containerFields parameter.

The 3rd parameter (2) passed to api.getFluxImages has been incorrect for a while now.

Fixes https://github.com/weaveworks/service-ui/issues/2749

@aaron7 aaron7 requested review from foot and fbarl June 20, 2018 12:42
Copy link
Contributor

@leth leth left a comment

Choose a reason for hiding this comment

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

Sorry, I think that one may have been leftover from some cleanup I did

@aaron7 aaron7 merged commit e5d881a into master Jun 20, 2018
@aaron7 aaron7 deleted the api-getfluximages-fix branch June 20, 2018 12:48
@bricef
Copy link
Contributor

bricef commented Jun 20, 2018

History

The history of this bug is very interesting.

First, in service-ui

commit 89c94df676a3eba6f9319b66a4c093477c70e32d
Date:   Tue Jul 4 17:13:10 2017 +0200

Adds a third version argument to the getFluxImages function, which remains unused from the scope code.

A little later, in scope

commit 7dcdc8db12d8b2b2f7c8334531c6be5855acac84
Date:   Wed Oct 11 12:41:18 2017 +0200

Actually uses the third argument to pass on version=2 to the function

After some reworking, The API function looses the third argument altogether in April this year

commit 4d4c60615acbd9a86eaf0439026b5ec6853a5c5f
Date:   Tue Apr 10 14:16:08 2018 +0100

The scope code isn't updated, but because javascript don't care, everything works just fine.

A little later, however, the service-ui code gets update to accept a third positional argument again in

commit 28f9cc04f66aeb14f72be92a2cad48e0963dd83e
Date:   Fri Jun 15 16:45:58 2018 +0100

The scope code, unaware, is still using the third positional argument as a version, but the service-ui code now expects an array.

💥BOOM💥

Lessons

  1. Don't use javascript, because it's too permissive? 🤷 Arity errors should be a thing godammit.
  2. Grep the entire codebase when refactoring service-ui for random snippets that might need updating.
  3. Anything else?

@leth
Copy link
Contributor

leth commented Jun 20, 2018

We could come up with another way to inject weave cloud functionality into scope so that the code which depends on the Weave Cloud UI stays in that codebase

@fbarl
Copy link
Contributor

fbarl commented Jun 20, 2018

We could come up with another way to inject weave cloud functionality into scope so that the code which depends on the Weave Cloud UI stays in that codebase

@foot and I had a lot of casual chat recently on how this could be done, maybe it was about time I opened an issue - https://github.com/weaveworks/service-ui/issues/2759 :)

@jml
Copy link
Contributor

jml commented Jun 25, 2018

The history confuses me. IIUC, the commit that breaks things is from Jun 15. However, the incident did not occur until Jun 20. What explains the discrepancy?

@leth
Copy link
Contributor

leth commented Jun 25, 2018

Commit 28f9cc04f66aeb14f72be92a2cad48e0963dd83e from June 15th was not merged until the afternoon of Monday June 18th. We locked the UI service on the 18th due to other issues so this may have hit production on the afternoon of the 19th

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.

5 participants