Skip to content

fix: offset all Argo CD sync-wave values by +31 to ensure positive waves#109

Open
minmzzhang wants to merge 4 commits intovalidatedpatterns:mainfrom
minmzzhang:update-sync-wave
Open

fix: offset all Argo CD sync-wave values by +31 to ensure positive waves#109
minmzzhang wants to merge 4 commits intovalidatedpatterns:mainfrom
minmzzhang:update-sync-wave

Conversation

@minmzzhang
Copy link
Copy Markdown
Collaborator

The Validated Patterns operator now applies the Argo CD super-role (ClusterRole) later in the deployment lifecycle. Resources annotated with negative sync-wave values could attempt to sync before the role existed, causing failures.

This adds +31 to every argocd.argoproj.io/sync-wave annotation across all charts, values-hub.yaml, and docs. Relative ordering is preserved; the smallest wave is now 1 (was -30). Inline comments referencing old wave numbers are updated to match.

See docs/SYNC-WAVE-INVENTORY.md for a full old/current mapping table.

@p-rog
Copy link
Copy Markdown
Collaborator

p-rog commented Mar 23, 2026

Overall it looks good to me. Let me test it.

@p-rog
Copy link
Copy Markdown
Collaborator

p-rog commented Mar 23, 2026

I tested this approach in a brand new cluster and it worked well. No errors and applications are synced without issues.

@mlorenzofr mlorenzofr self-requested a review March 23, 2026 15:36
Copy link
Copy Markdown
Collaborator

@mlorenzofr mlorenzofr left a comment

Choose a reason for hiding this comment

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

lgtm

@sabre1041
Copy link
Copy Markdown
Collaborator

@minmzzhang with #106 merged, a few issues have cropped up:

  1. Conflicts in this PR
  2. Managing the sync waves within individual resources. Now that these charts have been externalized, there is less control that we are/should have.

@minmzzhang
Copy link
Copy Markdown
Collaborator Author

@minmzzhang with #106 merged, a few issues have cropped up:

  1. Conflicts in this PR
  2. Managing the sync waves within individual resources. Now that these charts have been externalized, there is less control that we are/should have.

@sabre1041 great catch! Yes there are 3 charts, rhbk, compliance-scanning and quay, have sync-wave order defined. Hopefully this will be a one shot change then we won't touch the sync-wave very often. I've created PRs for updating those charts which are under review, also local commit is ready to solved the merge conflicts, will push when the charts are updated.

@p-rog
Copy link
Copy Markdown
Collaborator

p-rog commented Mar 31, 2026

@minmzzhang with #106 merged, a few issues have cropped up:

  1. Conflicts in this PR
  2. Managing the sync waves within individual resources. Now that these charts have been externalized, there is less control that we are/should have.

@sabre1041 great catch! Yes there are 3 charts, rhbk, compliance-scanning and quay, have sync-wave order defined. Hopefully this will be a one shot change then we won't touch the sync-wave very often. I've created PRs for updating those charts which are under review, also local commit is ready to solved the merge conflicts, will push when the charts are updated.

@minmzzhang please let us know once you fix the charts/compliance-scanning/, charts/compliance-scanning/, charts/compliance-scanning/ and also values-hub.yaml respectively. I think the docs/SYNC-WAVE-INVENTORY.md should also be updated.

@minmzzhang
Copy link
Copy Markdown
Collaborator Author

@minmzzhang with #106 merged, a few issues have cropped up:

  1. Conflicts in this PR
  2. Managing the sync waves within individual resources. Now that these charts have been externalized, there is less control that we are/should have.

@sabre1041 great catch! Yes there are 3 charts, rhbk, compliance-scanning and quay, have sync-wave order defined. Hopefully this will be a one shot change then we won't touch the sync-wave very often. I've created PRs for updating those charts which are under review, also local commit is ready to solved the merge conflicts, will push when the charts are updated.

@minmzzhang please let us know once you fix the charts/compliance-scanning/, charts/compliance-scanning/, charts/compliance-scanning/ and also values-hub.yaml respectively. I think the docs/SYNC-WAVE-INVENTORY.md should also be updated.

@p-rog yes the dosc/SYNC-WAVE-INVENTORY.md will be also updated accordingly.

@p-rog
Copy link
Copy Markdown
Collaborator

p-rog commented Apr 1, 2026

@minmzzhang maybe instead of adding more to the exclusions scope in the ACS Runtime custom policies, let's merge #116 update first like we discussed it today and then your issue will be fixed.

values-hub.yaml Outdated
path: charts/acs-policies
annotations:
argocd.argoproj.io/sync-wave: "51"
ignoreDifferences:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

With @sabre1041 we discussed usage of the ignoreDifferences earlier and we agreed that it's not the best approach. Using ignoreDifferences would work but treats the symptom rather than address the root cause. Hence it's better to fix the helm charts.

The Validated Patterns operator now applies the Argo CD super-role
(ClusterRole) later in the deployment lifecycle. Resources annotated
with negative sync-wave values could attempt to sync before the role
existed, causing failures.

This adds +31 to every argocd.argoproj.io/sync-wave annotation across
all charts, values-hub.yaml, and docs. Relative ordering is preserved;
the smallest wave is now 1 (was -30). Inline comments referencing old
wave numbers are updated to match.

See docs/SYNC-WAVE-INVENTORY.md for a full old/current mapping table.

Signed-off-by: Min Zhang <minzhang@redhat.com>
- Add explicit sync-wave to all active applications in values-hub.yaml
- Add sync-wave 48 to commented supply-chain application
- Wait for MachineConfigPool rollout before vault-config-jwt
- Update SYNC-WAVE-INVENTORY with unified deployment timeline

Signed-off-by: Min Zhang <minzhang@redhat.com>
…stergroup chart

Apply the same +31 sync-wave offset convention to values-coco-dev.yaml
so all waves are positive, matching values-hub.yaml. Revert the
temporary clusterGroupChartVersion pin (0.9.45 -> 0.9.*) since
positive-only waves eliminate the issue that prompted it.

Signed-off-by: Min Zhang <minzhang@redhat.com>
@@ -56,17 +56,17 @@ clusterGroup:
# targetNamespace: openshift-storage
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@minmzzhang It looks like the configurations associated with externalizing the Helm charts have not been incorporated into this cluster group

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@sabre1041 sorry my bad, should have checked this file thoroughly. Updated.

Replace local path references with externalized chart names to match
values-hub.yaml: compliance-scanning, rh-cert-manager, ztwim,
and commented-out rh-keycloak and quay-registry.

Signed-off-by: Min Zhang <minzhang@redhat.com>
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.

4 participants