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

fix(aa): refactor Automated Analysis Recording Setting #1014

Merged
merged 10 commits into from
May 5, 2023
Merged

fix(aa): refactor Automated Analysis Recording Setting #1014

merged 10 commits into from
May 5, 2023

Conversation

maxcao13
Copy link
Member

@maxcao13 maxcao13 commented May 3, 2023 •
edited
Loading

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed the last commit: git commit --amend --signoff

Fixes: #1006

Description of the change:

This puts the current configuration as a expandable panel in the AA recording config settings and allows the user to explicitly edit the configuration. It also prevents null templates from being set as a default configuration.

Motivation for the change:

See #1006

@maxcao13 maxcao13 added chore Refactor, rename, cleanup, etc. backport labels May 3, 2023
@maxcao13 maxcao13 requested review from andrewazores and tthvo May 3, 2023 21:45
@mergify mergify bot added the safe-to-test label May 3, 2023
@github-actions
Copy link

github-actions bot commented May 3, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1014-bcdf3827fe85293d13a97d66aab8da160adc453f sh smoketest.sh

@github-actions
Copy link

github-actions bot commented May 3, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1014-c26de9f5e025edaba5e5ac44d4a9506c83a24c5b sh smoketest.sh

@tthvo tthvo marked this pull request as draft May 3, 2023 23:30
@github-actions
Copy link

github-actions bot commented May 4, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1014-9177d5e7799fa324abae16f0dff2af7a47151bfe sh smoketest.sh

@maxcao13 maxcao13 marked this pull request as ready for review May 4, 2023 08:19
@github-actions
Copy link

github-actions bot commented May 4, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1014-b7b73fe892f929f0a71669b3a62d0e628d4741c0 sh smoketest.sh

tthvo
tthvo reviewed May 4, 2023
View reviewed changes
Copy link
Member

@tthvo tthvo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

UI looks good!

Not sure if its just me. The editing panel seems crashing in Automated Analysis Drawer -> click on the edit icon just closes the drawer panel.

I think it is the issue with TargetSelect trying to set global selection. This causes the AutomatedAnalysisCard to generateReport -> loading state -> closes the drawer.

See #1014 (comment)

tthvo
tthvo reviewed May 4, 2023
View reviewed changes
locales/en/public_old.json Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented May 4, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1014-89239a745c9117972e0c45fad467de87747f5b45 sh smoketest.sh

tthvo
tthvo reviewed May 4, 2023
View reviewed changes
Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

Some more comments and prettier needs running :))

maxcao13 added 4 commits May 4, 2023 17:18
@maxcao13
show current config
053e16b 
Signed-off-by: Max Cao <macao@redhat.com>
@maxcao13
prevent setting empty template
67d0864 
Signed-off-by: Max Cao <macao@redhat.com>
@maxcao13
add reset
cecb450 
Signed-off-by: Max Cao <macao@redhat.com>
@maxcao13
fix some handling
214678e 
Signed-off-by: Max Cao <macao@redhat.com>
maxcao13 added 5 commits May 4, 2023 17:18
@maxcao13
redesign
58f1d98 
Signed-off-by: Max Cao <macao@redhat.com>
@maxcao13
redesign 2
3f85c52 
Signed-off-by: Max Cao <macao@redhat.com>
@maxcao13
update tests and add confirmation
9a41676 
Signed-off-by: Max Cao <macao@redhat.com>
@maxcao13
re: fixes
847cc23 
Signed-off-by: Max Cao <macao@redhat.com>
@maxcao13
fix tests
fb3ded1 
Signed-off-by: Max Cao <macao@redhat.com>
@github-actions
Copy link

github-actions bot commented May 5, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1014-fb3ded1071be0d1ba21e47869ae5b40e18b2a38a sh smoketest.sh

tthvo
tthvo previously approved these changes May 5, 2023
View reviewed changes
Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

Looks awesome! Thanks!

@maxcao13
Copy link
Member Author

maxcao13 commented May 5, 2023

@andrewazores any other comments?

andrewazores
andrewazores reviewed May 5, 2023
View reviewed changes
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

Looks good other than the two minor localization spots

@maxcao13
localize
40d2ba7 
Signed-off-by: Max Cao <macao@redhat.com>
@github-actions
Copy link

github-actions bot commented May 5, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1014-40d2ba7a27286d3c1f21486a9150f39677474313 sh smoketest.sh

@ebaron ebaron merged commit 093e003 into cryostatio:main May 5, 2023
mergify bot pushed a commit that referenced this pull request May 5, 2023
@maxcao13 @mergify
fix(aa): refactor Automated Analysis Recording Setting (#1014)
46f919a 
* show current config

Signed-off-by: Max Cao <macao@redhat.com>

* prevent setting empty template

Signed-off-by: Max Cao <macao@redhat.com>

* add reset

Signed-off-by: Max Cao <macao@redhat.com>

* fix some handling

Signed-off-by: Max Cao <macao@redhat.com>

* redesign

Signed-off-by: Max Cao <macao@redhat.com>

* redesign 2

Signed-off-by: Max Cao <macao@redhat.com>

* update tests and add confirmation

Signed-off-by: Max Cao <macao@redhat.com>

* re: fixes

Signed-off-by: Max Cao <macao@redhat.com>

* fix tests

Signed-off-by: Max Cao <macao@redhat.com>

* localize

Signed-off-by: Max Cao <macao@redhat.com>

---------

Signed-off-by: Max Cao <macao@redhat.com>
(cherry picked from commit 093e003)
@mergify mergify bot mentioned this pull request May 5, 2023
Merged
ebaron pushed a commit that referenced this pull request May 5, 2023
@mergify @maxcao13
fix(aa): refactor Automated Analysis Recording Setting (#1014) (#1018)
ea3055a 
* show current config

Signed-off-by: Max Cao <macao@redhat.com>

* prevent setting empty template

Signed-off-by: Max Cao <macao@redhat.com>

* add reset

Signed-off-by: Max Cao <macao@redhat.com>

* fix some handling

Signed-off-by: Max Cao <macao@redhat.com>

* redesign

Signed-off-by: Max Cao <macao@redhat.com>

* redesign 2

Signed-off-by: Max Cao <macao@redhat.com>

* update tests and add confirmation

Signed-off-by: Max Cao <macao@redhat.com>

* re: fixes

Signed-off-by: Max Cao <macao@redhat.com>

* fix tests

Signed-off-by: Max Cao <macao@redhat.com>

* localize

Signed-off-by: Max Cao <macao@redhat.com>

---------

Signed-off-by: Max Cao <macao@redhat.com>
(cherry picked from commit 093e003)

Co-authored-by: Max Cao <macao@redhat.com>
@maxcao13 maxcao13 deleted the aa-settings branch May 5, 2023 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tthvo tthvo tthvo approved these changes

@andrewazores andrewazores andrewazores approved these changes

Assignees
No one assigned
Labels
backport chore Refactor, rename, cleanup, etc. safe-to-test
Projects
No open projects
2.3.0 release
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Task] Redesign Automated Analysis Recording Setting
4 participants
@maxcao13 @andrewazores @tthvo @ebaron