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

fix(events): Add dynamic config service to send events #724

Merged
merged 1 commit into from
May 5, 2020

Conversation

marchello2000
Copy link
Contributor

It's not currently possible to start/stop igor from sending events of completed builds/newly published images to echo.
It is useful to have this functionality because igor should always be "polling" to have a local cache of "triggered" builds/images.

This change allows an operator to:

  • have two clusters of igor both polling but only one sending events (i.e. triggering pipeline)
  • switch which cluster is sending events without generating duplicate events/double triggering

It's not currently possible to start/stop `igor` from sending events of completed builds/newly published images to `echo`.
It is useful to have this functionality because igor should always be "polling" to have a local cache of "triggered" builds/images.

This change allows an operator to:
* have two clusters of `igor` both polling but only one sending events (i.e. triggering pipeline)
* switch which cluster is sending events without generating duplicate events/double triggering
@@ -208,6 +219,8 @@ protected void internalPollSingle(PollContext ctx) {
.set(0);
}

sendEvents = sendEvents && isSendingEventsEnabled();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the actual change

@marchello2000 marchello2000 added the ready to merge Approved and ready for merge label May 5, 2020
@mergify mergify bot merged commit ec2ce88 into spinnaker:master May 5, 2020
@mergify mergify bot added the auto merged label May 5, 2020
@@ -264,6 +277,10 @@ public Long getLastPoll() {
return lastPoll.get();
}

private boolean isSendingEventsEnabled() {
return dynamicConfigService.isEnabled("igor.build.sendEvents", true);
Copy link
Contributor

@ajordens ajordens May 5, 2020

Choose a reason for hiding this comment

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

Not sure we need igor in here since the fp is going to be scoped to igor the application ... should the default be true or false? (I suppose true for backwards compatibility).

igor:
  build:
    sendEvents: true

The igor in that config snippet would also seem weird in igor.yml.

We do have some config under spinnaker.build tho?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spinnaker.build.sendEvents seems a bit weird though... but it would fit with convention - will switch to that.
def want to default to true, since that's the behavior today

Copy link
Contributor Author

Choose a reason for hiding this comment

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

marchello2000 added a commit to marchello2000/igor that referenced this pull request May 6, 2020
Follow up to feedback in spinnaker#724.
Renaming the property to `spinnaker.build.sendEventsEnabled`, this way it is symmetric to existing properties in `igor`, e.g.:

```yaml
spinnaker:
  build:
    pollingEnabled: true
    sendEventsEnabled: false
```
mergify bot pushed a commit that referenced this pull request May 6, 2020
Follow up to feedback in #724.
Renaming the property to `spinnaker.build.sendEventsEnabled`, this way it is symmetric to existing properties in `igor`, e.g.:

```yaml
spinnaker:
  build:
    pollingEnabled: true
    sendEventsEnabled: false
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants