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

Support for more complex message from manifest commit message to dispatch event. (To handle flux image automation where list images is set by default). Fix errors with deploys being marked failed prematurely. #39

Merged
merged 7 commits into from
Jan 7, 2023

Conversation

joshuadmatthews
Copy link
Contributor

@joshuadmatthews joshuadmatthews commented Nov 11, 2022

A few updates to handle using this with flux image automation, and also to resolve an issue where successful commits get marked as failed prematurely.

  1. Add try/except around extracting the runId and commitId from the source commit message, also passing along the unparsed commit message to support cases like flux image automation where there may be multiple image updates from commits/runs batched into a single manifest commit, and where it may be difficult to add a commit message formatted for extracting a runId/commitId.

  2. Changed DependencyNotReady mapping from error to pending. Once a commit status with error status is sent to github the commit itself goes into the failure status, and from there gitops-connector stop processing further messages for that commit. DependencyNotReady can be emitted during a deployment that is ultimately successful. I also think HealthCheckFailed may have the same issues, a health check may fail as a container is initializing and then ultimately end up succeeding so we wouldn't want a single failure to cause a failed commit I don't think?

This discussion from flux maintainer suggests that DependencyNotReady just means reconciliation is in progress and not an indication of failure.
fluxcd/flux2#1160

Also I am unsure how to test this in my own cluster ahead of time, so just wanted to call out that these changes haven't been tested live but I believe they are simple enough.

don't react to flux metadata.commit_status: 'updated' events, we only want to react the real events, and these are sent on every reconciliation even if nothing happened, so they aren't real.
Ignore errors if the source commit message can't be parse for the runid and commitId, also pass along the full commit message to allow for more complex scenarios.
Once we send error state to github, the commit status becomes failed and gitops connector will no longer update the commit. DependencyNotReady shouldn't mark things as failed as the deployment is still in progress.
@joshuadmatthews
Copy link
Contributor Author

joshuadmatthews commented Nov 11, 2022

@microsoft-github-policy-service agree company="Beeline"

@joshuadmatthews joshuadmatthews changed the title Patch 1 Support for more complex message from manifest commit message to dispatch event. (To handle flux image automation where list images is set by default). Fix errors with deploys being marked failed prematurely. Nov 14, 2022
@joshuadmatthews
Copy link
Contributor Author

@eedorenko are you available to review?

@joshuadmatthews
Copy link
Contributor Author

joshuadmatthews commented Nov 28, 2022

I have figured out a way to avoid DependencyNotReady marking the commit as failure by filtering that event out of my flux alerts, this is less than ideal though IMO I would rather the event be sent just not to mark the commit as failure.

apiVersion: notification.toolkit.fluxcd.io/v1beta1
kind: Alert
metadata:
  name: gitops-connector
  namespace: flux-system
spec:
  eventSeverity: info
  eventSources:
  - kind: GitRepository
    name: myrepo
  - kind: Kustomization
    name: mykustomization
  providerRef:
    name: gitops-connector
  exclusionList:
    - "Dependencies do not meet ready condition"

I have not been able to work around the runID commitID parsing since it parses them based on "/" characters and flux image automation provides only a list of images that were updated as the commit message and they contains a "/" with no way for me to remove it.

@eedorenko
Copy link
Collaborator

lgtm

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.

2 participants