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

feat(RELEASE-1246): make publish-to-cgw task idempotent #781

Draft
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

seanconroy2021
Copy link
Member

Describe your changes

This update makes the publish-to-cgw task idempotent by checking if files already exist under the specified product name and version before processing. If files are present, they are skipped to avoid errors. Additionally, the pubtools-content-gateway command has been removed and we are now calling the CGW api directly.

Relevant Jira

CGW Docs

Checklist before requesting a review

  • I have marked as draft or added do not merge label if there's a dependency PR
    • If you want reviews on your draft PR, you can add reviewers or add the release-service-maintainers handle if you are unsure who to tag
  • [* ] My commit message includes Signed-off-by: My name <email>
  • [ *] I have bumped the task/pipeline version string and updated changelog in the relevant README
  • [ *] I read CONTRIBUTING.MD and commit formatting

Copy link

openshift-ci bot commented Jan 22, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@seanconroy2021
Copy link
Member Author

I still need to add my tests, but I'm nearly there. 😌

@johnbieren johnbieren requested a review from a team January 22, 2025 20:23
This update makes the task idempotent by checking
if files already exist under the specified product
name and version before processing. If files are
present, they are skipped to avoid errors.
Additionally, the pubtools-content-gateway
command has been removed and we are now
directly calling the CGW api.

Signed-off-by: Sean Conroy <[email protected]>
@johnbieren
Copy link
Collaborator

@pkhander @swickersh can you take a look at this change please?

Copy link
Collaborator

@johnbieren johnbieren left a comment

Choose a reason for hiding this comment

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

It looks sane to me, but you will need to add test(s) for this change I think

@@ -16,6 +15,12 @@ Tekton task to publish content to Red Hat's Developer portal using pubtools-cont
| cgwHostname | The hostname of the content-gateway to publish the metadata to | yes | https://developers.redhat.com/content-gateway/rest/admin |
| cgwSecret | The kubernetes secret to use to authenticate to content-gateway | yes | publish-to-cgw-secret |

## Changes in 0.3.0
* Make the task idempotent by checking if files
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/if files/if files are/

@pkhander
Copy link
Contributor

I would like to point out that there is another (copy) script that interacts with CGW for another pipeline - https://github.com/konflux-ci/release-service-utils/blob/main/developer-portal-wrapper/developer_portal_wrapper.py

Please keep that in mind with this work. Not ALL publishing to cgw will be idempotent when this is merged.

Ideally just the developer_portal_wrapper should be updated and used inside this task but I will let you decide the implementation details.

@mmalina
Copy link
Contributor

mmalina commented Jan 24, 2025

I would like to point out that there is another (copy) script that interacts with CGW for another pipeline - https://github.com/konflux-ci/release-service-utils/blob/main/developer-portal-wrapper/developer_portal_wrapper.py

Please keep that in mind with this work. Not ALL publishing to cgw will be idempotent when this is merged.

Ideally just the developer_portal_wrapper should be updated and used inside this task but I will let you decide the implementation details.

Not having Python code directly in the task script would also help with testing - when Python is used directly, it's not possible to use our task testing framework and you can't do Python unit testing this way either. So I agree that switching to developer_portal_wrapper sounds like a good idea.

@seanconroy2021
Copy link
Member Author

I would like to point out that there is another (copy) script that interacts with CGW for another pipeline - https://github.com/konflux-ci/release-service-utils/blob/main/developer-portal-wrapper/developer_portal_wrapper.py
Please keep that in mind with this work. Not ALL publishing to cgw will be idempotent when this is merged.
Ideally just the developer_portal_wrapper should be updated and used inside this task but I will let you decide the implementation details.

Not having Python code directly in the task script would also help with testing - when Python is used directly, it's not possible to use our task testing framework and you can't do Python unit testing this way either. So I agree that switching to developer_portal_wrapper sounds like a good idea.

Sweet, I will just move it over there, then ... and use pytest
I did find an interesting way to test in mock.sh by mocking each request, e.g. /products

@swickersh
Copy link
Contributor

Sweet, I will just move it over there, then ... and use pytest

It wont be that simple. The two have deviated slightly.
For example, the matching_component logic, the short_url_prefix, argparse, etc.

developer_portal_wrapper.py was created as a modified version of the python section nested in this task to work with rhel_ai. We want to be careful not to break either workflow when combining these.

IMO, you're running into some scope creep here. You might ask to increase your jira story points and allow some capacity for testing. Maybe these should be separate commits?

Alternatively, you could make another developer_portal_wrapper.py (slight different name) to work with this workflow. Keep the existing one for rhel-ai to continue utilizing. Then in a future Jira we can make a unified script that works with both.

@mmalina @pkhander wdyt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants