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 new ADMIN endpoint to import execution #3310

Merged
merged 3 commits into from
Nov 25, 2019
Merged

feat(executions): Adding new ADMIN endpoint to import execution #3310

merged 3 commits into from
Nov 25, 2019

Conversation

srekapalli
Copy link
Contributor

@srekapalli srekapalli commented Nov 21, 2019

Adding new Admin endpoint to import execution from another spinnaker instance.

Useful to get a execution into other envs for troubleshooting.
Only imports executions with TERMINAL, SUCCEEDED or CANCELLED states.

…r spinnaker instance.

Useful to get a execution into other envs for troubleshooting.
Only imports executions with TERMINAL, SUCCEEDED or CANCELLED states.
@marchello2000
Copy link
Contributor

Thanks for doing this, I think it would be useful and LGTM, but would like another opinion as to any side effects this could cause, @cfieber / @robzienert ?

@marchello2000 marchello2000 changed the title feat(executions): Adding new endpoint to import execution from anothe… feat(executions): Adding new ADMIN endpoint to import execution Nov 21, 2019
Copy link
Contributor

@marchello2000 marchello2000 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@marchello2000 marchello2000 added the ready to merge Approved and ready for merge label Nov 25, 2019
@mergify mergify bot added the auto merged Merged automatically by a bot label Nov 25, 2019
@mergify mergify bot merged commit da46413 into spinnaker:master Nov 25, 2019
@ResponseStatus(HttpStatus.CREATED)
Map<String, String> createExecution(@RequestBody Execution execution) {

if (front50Service && !front50Service.get(execution.application)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe front50Service.get() will raise an exception if the application doesn't exist (vs. returning null).

I'm not sure we want to import executions for applications that don't exist?


try {
executionRepository.retrieve(execution.type, execution.id)
log.warn('Execution found with id: []', execution.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

warn seems like the wrong logging level? debug maybe? (or perhaps it's not even relevant given it's going to raise an exception immediately)

log.warn('Execution found with id: []', execution.id)
throw new InvalidRequestException('Execution already exists with id: ' + execution.id)
} catch(ExecutionNotFoundException e) {
log.info('Execution not found .. can import it..')
Copy link
Contributor

Choose a reason for hiding this comment

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

This log message seems irrelevant and would get lost in the noise? There's no identifier included in the output.

sergiopena pushed a commit to sergiopena/orca that referenced this pull request Dec 13, 2019
…naker#3310)

* feat(executions): Adding new endpoint to import execution from another spinnaker instance.

Useful to get a execution into other envs for troubleshooting.
Only imports executions with TERMINAL, SUCCEEDED or CANCELLED states.

* Disallow if execution already exists and also add some info helper messages
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 Approved and ready for merge target-release/1.18
Projects
None yet
4 participants