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

feat(executions): Adding redirect for execution without application #7076

Merged
merged 6 commits into from
Jun 4, 2019

Conversation

alanmquach
Copy link
Contributor

Redirects from /#/executions/ABC123 to /#/applications/egg/executions/details/ABC123.

name: 'executionLookup',
url: '/executions/:executionid',
params: {
executionid: { dynamic: true },
Copy link
Contributor

Choose a reason for hiding this comment

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

why not camel case this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like there was some unspoken rule from the ancient nodejs days where path params were all lowercase?

In other words, idk so let me just camel case it.

Copy link
Contributor

@anotherchrisberry anotherchrisberry left a comment

Choose a reason for hiding this comment

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

Love it, only question is parameter name

@christopherthielen
Copy link
Contributor

I would prefer to see this as a redirectTo on the state definition, and no ui component doing the work.

return undefined;
}

return Promise.resolve()
Copy link
Contributor

Choose a reason for hiding this comment

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

can this Promise.resolve() be removed?

        return executionService.getExecution(executionId)
          .then(execution =>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

redirectTo expects a Promise so needed to convert the IPromise from executionService.

Is this the right way to do that or is there a better way?

Copy link
Contributor

@christopherthielen christopherthielen left a comment

Choose a reason for hiding this comment

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

nice work

@christopherthielen christopherthielen merged commit 3f21c1f into spinnaker:master Jun 4, 2019
christopherthielen added a commit to christopherthielen/deck that referenced this pull request Jun 5, 2019
8398d77 refactor(*): make accountExtractor return an array of strings (spinnaker#7068)
e6d1586 feat(core/presentation): support JSX for validationMessage in FormFields (spinnaker#7083)
78a9f72 fix(core): Disable rewriteLinks to allow proper event handling (spinnaker#7064)
3f21c1f feat(executions): Adding redirect for execution without application (spinnaker#7076)
f89d97d feat(core/presentation): Add "success" type to ValidationMessage (spinnaker#7082)
4e89a39 refactor(core/presentation): Consolidate Checklist and ChecklistInput components (spinnaker#7077)
9bbc002 feat(core/presentation): Support inline radio buttons (spinnaker#7078)
b142789 fix(triggers): Poll on execution id instead of event id (spinnaker#7067)
6af93e3 refactor(pipeline): Pipeline stage execution details to react (spinnaker#7063)
christopherthielen added a commit that referenced this pull request Jun 5, 2019
8398d77 refactor(*): make accountExtractor return an array of strings (#7068)
e6d1586 feat(core/presentation): support JSX for validationMessage in FormFields (#7083)
78a9f72 fix(core): Disable rewriteLinks to allow proper event handling (#7064)
3f21c1f feat(executions): Adding redirect for execution without application (#7076)
f89d97d feat(core/presentation): Add "success" type to ValidationMessage (#7082)
4e89a39 refactor(core/presentation): Consolidate Checklist and ChecklistInput components (#7077)
9bbc002 feat(core/presentation): Support inline radio buttons (#7078)
b142789 fix(triggers): Poll on execution id instead of event id (#7067)
6af93e3 refactor(pipeline): Pipeline stage execution details to react (#7063)
@alanmquach alanmquach deleted the redirector branch July 10, 2019 23:58
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.

4 participants