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.

Sign up for GitHub

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

Jump to bottom

feat(hydrator): enable controller #19539

Closed
wants to merge 2 commits into from
Closed

feat(hydrator): enable controller #19539

wants to merge 2 commits into from

Conversation

crenshaw-dev
Copy link
Member

I'm breaking the hydrator into separate PRs. This is party to make them easier to review, partly because the commit history on the original PR is pretty messy at this point.

This PR is for the changes to the app controller to process the new sourceHydrator API + necessary tests.

I've added everyone who helped as a co-author on this PR.

@todaywasawesome todaywasawesome added the hydrator Issues related to the manifest hydrator. label Aug 15, 2024
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch from 6ce7dc0 to e67e3ef Compare September 19, 2024 19:52
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch 2 times, most recently from b642ed7 to 1168d9d Compare October 1, 2024 17:14
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch from f5e91ef to 40cdcf2 Compare October 11, 2024 15:19
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch from 40cdcf2 to d4fb401 Compare October 11, 2024 23:19
@crenshaw-dev crenshaw-dev force-pushed the commit-server branch 2 times, most recently from 2d34f7f to 5cc7db9 Compare October 17, 2024 21:07
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch from d4fb401 to 89bfa36 Compare October 17, 2024 21:26
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch 3 times, most recently from 619f45c to 3640971 Compare October 25, 2024 02:47
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch 2 times, most recently from ccc9e4b to d1e0886 Compare October 31, 2024 21:21
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch from d1e0886 to d233aad Compare October 31, 2024 22:20
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch from d233aad to a9461ac Compare November 3, 2024 18:12
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch 3 times, most recently from bcf1fb4 to 7d8965c Compare November 4, 2024 21:02
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch from 7d8965c to 2f28892 Compare November 21, 2024 18:32
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch 3 times, most recently from e4a9061 to b19beec Compare November 21, 2024 21:21
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch 3 times, most recently from b8eedba to ccb3553 Compare December 6, 2024 17:42
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch from ccb3553 to 7ee0ce0 Compare December 9, 2024 20:34
agaudreault
agaudreault reviewed Dec 11, 2024
View reviewed changes
cmd/argocd/commands/app_test.go Show resolved Hide resolved
controller/appcontroller.go Outdated Show resolved Hide resolved
docs/operator-manual/argocd-cmd-params-cm.yaml Outdated Show resolved Hide resolved
manifests/namespace-install-with-hydrator/kustomization.yaml Outdated Show resolved Hide resolved
server/application/application_test.go
@@ -109,7 +109,7 @@ func fakeResolveRevisionResponseHelm() *apiclient.ResolveRevisionResponse {

func fakeRepoServerClient(isHelm bool) *mocks.RepoServerServiceClient {
mockRepoServiceClient := mocks.RepoServerServiceClient{}
mockRepoServiceClient.On("ListApps", mock.Anything, mock.Anything).Return(fakeAppList(), nil)
mockRepoServiceClient.On("GetProcessableApps", mock.Anything, mock.Anything).Return(fakeAppList(), nil)
Copy link
Member

Choose a reason for hiding this comment

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

This change seems unrelated to this PR 🤔

util/db/repository.go
Comment on lines +225 to +229
// TODO: enrich with write credentials.
//if err := db.enrichCredsToRepo(ctx, repository); err != nil {
// return repository, fmt.Errorf("unable to enrich write repository %q info with credentials: %w", repoURL, err)
//}

Copy link
Member

Choose a reason for hiding this comment

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

Forgotten? Not a blocker, but create an issue if it is intended to implement later

Copy link
Member Author

Choose a reason for hiding this comment

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

Added docs, will check for an issue...

controller/hydrator/hydrator.go Outdated Show resolved Hide resolved
leoluz
leoluz reviewed Dec 11, 2024
View reviewed changes
util/db/write_repository.go Outdated Show resolved Hide resolved
util/db/write_repository.go Outdated Show resolved Hide resolved
util/db/repository_secrets.go
@@ -21,6 +21,8 @@ var _ repositoryBackend = &secretsRepositoryBackend{}

type secretsRepositoryBackend struct {
db *db
// If true, the backend will manage write only credentials. If false, it will manage only read credentials.
writeCreds bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally this should be a parameter passed to getSecretType(). Not sure I understand if there is a real necessity to have it in the secretsRepositoryBackend state.

@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch 2 times, most recently from 4dece23 to 8c61044 Compare December 12, 2024 14:29
@crenshaw-dev
Copy link
Member Author

@agaudreault @ashutosh16 @leoluz I addressed the easier comments, still working on the more involved ones.

@crenshaw-dev crenshaw-dev force-pushed the commit-server branch 2 times, most recently from bd22b69 to 27eef93 Compare December 13, 2024 21:05
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch 2 times, most recently from 336896b to ac5d698 Compare December 13, 2024 22:56
@crenshaw-dev
feat(hydrator): enable controller
fed8833 
Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
Co-authored-by: Omer Azmon <omer_azmon@intuit.com>
Co-authored-by: daengdaengLee <gunho1020@gmail.com>
Co-authored-by: Juwon Hwang (Kevin) <juwon8891@gmail.com>
Co-authored-by: thisishwan2 <feel000617@gmail.com>
Co-authored-by: mirageoasis <kimhw0820@naver.com>
Co-authored-by: Robin Lieb <robin.j.lieb@gmail.com>
Co-authored-by: miiiinju1 <gms07073@ynu.ac.kr>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

feat(hydrator): enable controller

Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
Co-authored-by: Omer Azmon <omer_azmon@intuit.com>
Co-authored-by: daengdaengLee <gunho1020@gmail.com>
Co-authored-by: Juwon Hwang (Kevin) <juwon8891@gmail.com>
Co-authored-by: thisishwan2 <feel000617@gmail.com>
Co-authored-by: mirageoasis <kimhw0820@naver.com>
Co-authored-by: Robin Lieb <robin.j.lieb@gmail.com>
Co-authored-by: miiiinju1 <gms07073@ynu.ac.kr>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

allow opt-in

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

separation between app controller and hydrator

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

simplify diff

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

todos

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

simplify

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

add dry sha to logs

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

add app name to logs

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

more logging, no caching

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

fix cluster install

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

don't interrupt an ongoing hydrate operation

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

revert hydrate loop fix

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

handle project-scoped repo creds

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

codegen

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

improve docs

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

fixes from comments

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
@crenshaw-dev
set hydrator enabled key when using hydrator manifests
bd35d3d 
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

fix manifests

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
@crenshaw-dev crenshaw-dev force-pushed the hydrator-controller-changes branch from ac5d698 to bd35d3d Compare December 14, 2024 17:48
@crenshaw-dev
Copy link
Member Author

Parent PR was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agaudreault agaudreault agaudreault left review comments

@ashutosh16 ashutosh16 ashutosh16 left review comments

@leoluz leoluz leoluz left review comments

Assignees
No one assigned
Labels
hydrator Issues related to the manifest hydrator.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@crenshaw-dev @leoluz @ashutosh16 @agaudreault @todaywasawesome