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(jira): Add Jira issue transition to JiraNotificationService #1080

Merged
merged 3 commits into from
Feb 4, 2021

Conversation

jonsie
Copy link
Contributor

@jonsie jonsie commented Feb 3, 2021

transitionContext payload can be the full Jira transition REST API payload, however the transition ID will be looked up based on the transition name


@Component
@ConditionalOnProperty("jira.enabled")
public class JiraNotificationService implements NotificationService {
private static final Logger LOGGER = LoggerFactory.getLogger(JiraNotificationService.class);
private static final int MAX_RETRY = 3;
private static final long RETRY_BACKOFF = 3;
private static final long RETRY_BACKOFF = 100;
Copy link
Contributor Author

@jonsie jonsie Feb 3, 2021

Choose a reason for hiding this comment

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

A retry backoff of 3ms seemed absurdly low, setting this to 100ms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Debatable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong opinions, just don't follow the point of sleeping for 3 milliseconds (my reasoning is it may as well be zero in that case). 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

My original comment was a joke, sorry ... I suspect the original was a big mistake.

Honestly, because it's a linear backoff and only 3 attempts, I'd probably do a couple seconds or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, I thought maybe you were joking but I totally couldn't tell 😂

kv("transitionRequestBody", transitionContext), errors(e)),
e);
}
}
Copy link
Contributor Author

@jonsie jonsie Feb 3, 2021

Choose a reason for hiding this comment

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

Error on invalid transition name looks like:

Failed to transition Jira issue EXMPL-0000: {errors={issue=EXMPL-0000, transitionName=Done, validTransitionNames=[Reopen, Reopen and start progress]}}

@jonsie jonsie force-pushed the add-jira-transition branch 3 times, most recently from 4b9a622 to 8b2f283 Compare February 3, 2021 08:00
`transitionContext` payload can be the full Jira transition REST API payload, however the transition ID will be looked up based on the transition name
Copy link
Contributor

@ajordens ajordens left a comment

Choose a reason for hiding this comment

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

Minor nits, by and large I think it's good.


@Component
@ConditionalOnProperty("jira.enabled")
public class JiraNotificationService implements NotificationService {
private static final Logger LOGGER = LoggerFactory.getLogger(JiraNotificationService.class);
private static final int MAX_RETRY = 3;
private static final long RETRY_BACKOFF = 3;
private static final long RETRY_BACKOFF = 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

Debatable.

@jonsie
Copy link
Contributor Author

jonsie commented Feb 3, 2021

Added option to comment on issue.

@ajordens
Copy link
Contributor

ajordens commented Feb 4, 2021

New object looks good to me (not sure if there's lombok in here and @DaTa would have been sufficed over get/set ... either way I'm ok).

@ajordens ajordens self-requested a review February 4, 2021 00:10
@jonsie jonsie merged commit 8898b68 into spinnaker:master Feb 4, 2021
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.

3 participants