Skip to content

Conversation

@ydayagi
Copy link
Contributor

@ydayagi ydayagi commented Jul 15, 2025

@masayag masayag requested a review from elai-shalev July 15, 2025 13:11
Copy link
Collaborator

@elai-shalev elai-shalev 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:

  1. Should we remove the deprecation notices from all docs and relevant files now that the deprecation has been performed?

  2. Were all templates tested after the changes? The templates can be unpredictable with changes, especially when changing the workflow in the skeleton projects

@@ -32,59 +30,54 @@ functions:
- name: assessmentResult
Copy link
Collaborator

Choose a reason for hiding this comment

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

About L18-30
So we're keeping the "SampleAssessment" java class, but only updating the workflowId and not "mimicing the assessment"? Did you consider renaming this action + Java class to have no "assessment" wording remaining?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i prefer not to rename or change the logic. i think this will be revised later on by motu and piotr and you and others. not sure which direction this will go. so better keep it simple.

@ydayagi
Copy link
Contributor Author

ydayagi commented Jul 16, 2025

Some more comments:

  1. Should we remove the deprecation notices from all docs and relevant files now that the deprecation has been performed?
  2. Were all templates tested after the changes? The templates can be unpredictable with changes, especially when changing the workflow in the skeleton projects

@elai-shalev about the deprecation notice we should ask @pkliczewski and @masayag and perhaps you have an opinion. what do u suggest?
For testing it officially I would be glad to receive instructions on how to test it. I saw a CI e2e test so I figured we are covered

FLPATH-2324
https://issues.redhat.com/browse/FLPATH-2324

Signed-off-by: Yaron Dayagi <ydayagi@redhat.com>
@ydayagi
Copy link
Contributor Author

ydayagi commented Jul 21, 2025

@elai-shalev @masayag

I removed the deprecation notice

@elai-shalev
Copy link
Collaborator

Hey @ydayagi
The CI passed but looking into the log itself I believe if might have not actually validates that the templates are correct.
looking at ADVANCED_TASK_ID is null in the log tells me that the template wasn't properly launched. Probably a falso positive of the CI.

To test the software templates, you need:

  1. Replace the catalog URLs in the RHDH catalog config in the Go-operator here to use the URLs to the templates in this PR. You can also just install the operator regularly, scale down rhdh, replate the catalog items in the configmap and redeploy rhdh.
  2. Your RHDH should be configured with all the necessary github/gitlab requirement. QE has a job to configure all of this automatically, you could ask them for a cluster/ use the testing cluster
  3. Once you have the correct new software templates in RHDH, run them through the catalog and make sure they work

@elai-shalev
Copy link
Collaborator

Also about the deprecation notice, I think we should remove all notices in a seperate PR once all code deprecation PRs are merged

@ydayagi
Copy link
Contributor Author

ydayagi commented Jul 21, 2025

@elai-shalev @masayag during planning we said the @elai-shalev will test software templates. so I think we can go ahead and merge

Copy link
Collaborator

@elai-shalev elai-shalev left a comment

Choose a reason for hiding this comment

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

LGTM, withstanding the templates were not tested thoroughly. If needed a patch/fix will be added.

@elai-shalev elai-shalev merged commit da5e818 into rhdhorchestrator:main Jul 22, 2025
1 check passed
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.

2 participants