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

fix: Project filter not applied to changed apps from other projs (#9651) #9700

Conversation

keithchong
Copy link
Contributor

@keithchong keithchong commented Jun 17, 2022

Signed-off-by: Keith Chong kykchong@redhat.com

Fixes #9651

Notes:

The app changes are being sent to the UI.

Consider:

  1. Create app1 in project1 and app2 in default project.
  2. Sync app2
  3. Immediately apply filter on project1
    --> When the app2 is 'modified' or changed, the application list does not contain app2, so the index is -1. Therefore the app is added to the list via the call to unshift (See loadApplications in applications-list.tsx)
    The fix then prevents the app from being added because the project filter has been enabled and the app's project is not in the filter.

Secondly, from the another browser window, create a new app3 in project1
--> app3 should show up in the original Argo CD instance, because app3's project is in the project filter.

Repeat the above steps with and without the fix, to verify this PR.

Note, from the application service, it looks like in sendIfPermitted, the intention was to not send the change event and application to the UI if the app's project is not part of the filter? Apparently, the UI is still receiving these events. Perhaps someone can look into that -- I could not see off-hand why. In the meantime, this could be a UI-only fix for this issue.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

@keithchong keithchong force-pushed the 9651-ProjectFilterNotAppliedToChangedAppsFromOtherProjects branch from d19fccc to 5b8a2b8 Compare June 17, 2022 01:59
@codecov
Copy link

codecov bot commented Jun 17, 2022

Codecov Report

Merging #9700 (5b8a2b8) into master (3966e50) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #9700      +/-   ##
==========================================
+ Coverage   45.76%   45.78%   +0.02%     
==========================================
  Files         225      226       +1     
  Lines       26565    26713     +148     
==========================================
+ Hits        12157    12231      +74     
- Misses      12751    12818      +67     
- Partials     1657     1664       +7     
Impacted Files Coverage Δ
server/application/websocket.go 8.33% <0.00%> (-4.57%) ⬇️
reposerver/cache/cache.go 60.52% <0.00%> (-3.76%) ⬇️
server/application/logs.go 82.45% <0.00%> (-3.26%) ⬇️
applicationset/generators/scm_provider.go 37.64% <0.00%> (-2.62%) ⬇️
pkg/apiclient/apiclient.go 0.38% <0.00%> (-1.25%) ⬇️
util/webhook/webhook.go 67.66% <0.00%> (-0.69%) ⬇️
pkg/apiclient/grpcproxy.go 0.00% <0.00%> (ø)
...plicationset/services/scm_provider/azure_devops.go 71.56% <0.00%> (ø)
reposerver/repository/repository.go 61.91% <0.00%> (+0.52%) ⬆️
...is/applicationset/v1alpha1/applicationset_types.go 36.95% <0.00%> (+2.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3966e50...5b8a2b8. Read the comment docs.

@crenshaw-dev
Copy link
Collaborator

@keithchong I think we can just use the server-side fix: #9709

@keithchong
Copy link
Contributor Author

Agreed, fix is in #9709. Closing.

@keithchong keithchong closed this Jun 17, 2022
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.

Project filter is not applied to refreshing applications from other projects
2 participants