-
Notifications
You must be signed in to change notification settings - Fork 13
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
ROX-26039: Create tenant argoCd app #2009
Conversation
cb7795a
to
2f08d57
Compare
f42e55c
to
a1ac500
Compare
a1ac500
to
3635f24
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
e3a4736
to
e4d10fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like being the "Process-driven block a PR because of formal issues guy", but for a change like this, I'd expect more information in the PR description. For instance how you've tested the changes and also a functional description of what you expect / not expect to work after this PR.
return nil | ||
} | ||
|
||
func (r *CentralReconciler) getArgoCDApplication(remoteCentral private.ManagedCentral) (*argocd.Application, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The method name was confusing me. I expected this function to send a GET requests to argoCd as opposed to creating the *argocd.Application
object from a remoteCentral.
You're right. After some thought, I believe it would make sense to merge both https://github.com/stackrox/acs-fleet-manager/pull/2074/files and this PR together. Otherwise it might break the local development setup (because argoCD CRDs would not be present). Though it is easier to review separately. Edit: suspicion confirmed by the failing e2e test.
|
e4d10fa
to
a614365
Compare
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johannes94, kovayur, ludydoo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
37ef489
to
478540f
Compare
/retest |
2 similar comments
/retest |
/retest |
Description
Adds the support for deploying tenant-resources through ArgoCD rather than the built-in
tenant-resources
chart. TheCentralReconciler
willEnvironment variables have been added that represent the default
tenant-resources
application source. Currently, it defaults togithub.com/stackrox/acscs-manifests
tenant-resources
HEAD
Because this is an opt-in feature, at this point deploying this will have no consequence on the int/stage/prod environments.
The
acscs-manifests
repository is private. But ArgoCD should have access to it thanks to https://github.com/stackrox/acs-fleet-manager-aws-config/pull/257A further step in the development of the feature is to enable the local development support for ArgoCD: https://github.com/stackrox/acs-fleet-manager/pull/2074/files
Checklist (Definition of Done)
Test manual
ROX-12345: ...
Discussed security and business related topics privately. Will move any security and business related topics that arise to private communication channel.Add secret to app-interface Vault or Secrets Manager if necessaryRDS changes were e2e tested manuallyCheck AWS limits are reasonable for changes provisioning new resourcesTest manual
TODO: Add manual testing efforts