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

remove duplicate workflow installation and deletion #99

Closed
wants to merge 34 commits into from

Conversation

ccfishk
Copy link
Contributor

@ccfishk ccfishk commented Feb 12, 2022

Signed-off-by: jhu02 [email protected]

Fixes:

  1. remove duplicate workflow installation
  2. remove duplicate workflow deletion
  3. make addon status resources completed

Changes:

  1. use k8s resources event (add/update/deletion) trigger addon installation/status update
  2. add code-generator comment marker to generate addon clientset/informers/listers addon API access
  3. refactor controller skeleton

Clarify the issue motivation as well as suggested solutions in following ticket as more small PRs could lose the original context.
https://github.intuit.com/kubernetes/arktika/issues/3541

Signed-off-by: jhu02 <[email protected]>
@ccfishk ccfishk requested a review from a team as a code owner February 12, 2022 19:42
Signed-off-by: jhu02 <[email protected]>
@ccfishk ccfishk changed the title [wip] fix perf based on load-test [wip] tune controller based on load-test Feb 12, 2022
@ccfishk ccfishk requested a review from tekenstam February 14, 2022 18:42
@ccfishk ccfishk changed the title [wip] tune controller based on load-test [wip] increase workflow total after switch to script template and load tests Feb 14, 2022
@ccfishk ccfishk changed the title [wip] increase workflow total after switch to script template and load tests increase workflow total after switch to script template and load tests Feb 15, 2022
@ccfishk ccfishk changed the title increase workflow total after switch to script template and load tests [wip] increase workflow total after switch to script template and load tests Feb 16, 2022
@ccfishk ccfishk changed the title [wip] increase workflow total after switch to script template and load tests [wip] reduce addon process cycle Feb 16, 2022
@ccfishk ccfishk changed the title [wip] reduce addon process cycle reduce addon process cycle Feb 17, 2022
@ccfishk ccfishk changed the title reduce addon process cycle through remove duplicate workflow installation and deletion [wip] reduce addon process cycle through remove duplicate workflow installation and deletion Feb 18, 2022
@ccfishk ccfishk changed the title [wip] reduce addon process cycle through remove duplicate workflow installation and deletion reduce addon process cycle through remove duplicate workflow installation and deletion Feb 19, 2022
@kevdowney
Copy link
Collaborator

kevdowney commented Feb 20, 2022

This and other PRs are too large, very hard to review thoroughly, here's my suggestion... let's break these changes up into multiple PRs:

  • Generate an Addon clientset (PR-#)
  • Switch to use Argo clientset (PR-#)
  • Etc. (PR-#)

These changes then can be quickly reviewed and merged.

@ccfishk
Copy link
Contributor Author

ccfishk commented Feb 20, 2022

This and other PRs are too large, very hard to review thoroughly, here's my suggestion... let's break these changes up into multiple PRs:

  • Generate an Addon clientset (PR-#)
  • Switch to use Argo clientset (PR-#)
  • Etc. (PR-#)

These changes then can be quickly reviewed and merged.

Yes, this is a good idea

PR #1
Generate API clientset/lister/informer and refer them accordingly
#111

@ccfishk
Copy link
Contributor Author

ccfishk commented Feb 22, 2022

It would be better transfer this PR's changes into several PRs for better understanding, convert this PR into WIP

@ccfishk ccfishk changed the title reduce addon process cycle through remove duplicate workflow installation and deletion [wip] reduce addon process cycle through remove duplicate workflow installation and deletion Feb 22, 2022
@ccfishk
Copy link
Contributor Author

ccfishk commented Feb 23, 2022

@kevdowney, and all, per requests, the PR needs split into several small PRs for better understanding.
at this stage, I will focus on address the comments in this PR before opening new PRs.
#111 (comment)

@ccfishk ccfishk marked this pull request as draft February 23, 2022 23:31
@ccfishk ccfishk changed the title [wip] reduce addon process cycle through remove duplicate workflow installation and deletion educe addon process cycle through remove duplicate workflow installation and deletion Feb 25, 2022
@ccfishk ccfishk marked this pull request as ready for review February 25, 2022 16:20
@ccfishk ccfishk changed the title educe addon process cycle through remove duplicate workflow installation and deletion reduce addon process cycle through remove duplicate workflow installation and deletion Feb 25, 2022
@ccfishk
Copy link
Contributor Author

ccfishk commented Feb 27, 2022

This and other PRs are too large, very hard to review thoroughly, here's my suggestion... let's break these changes up into multiple PRs:

  • Generate an Addon clientset (PR-#)
  • Switch to use Argo clientset (PR-#)
  • Etc. (PR-#)

These changes then can be quickly reviewed and merged.

after merging #116 to master
This PR will only have around 15 change files, not really a big PR any more.

@ccfishk ccfishk changed the title reduce addon process cycle through remove duplicate workflow installation and deletion remove duplicate workflow installation and deletion Feb 27, 2022
@ccfishk
Copy link
Contributor Author

ccfishk commented Mar 1, 2022

The conversations of this PR has been resolved. And this PR is moved to #118.

@ccfishk ccfishk closed this Mar 1, 2022
@kevdowney kevdowney deleted the tests-and-perf branch May 1, 2023 16:56
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.

3 participants