Skip to content

Fix ODH notebooks sync workflow #574

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

Conversation

abhijeet-dhumal
Copy link
Contributor

@abhijeet-dhumal abhijeet-dhumal commented Jun 25, 2024

Issue link

RHOAIENG-9027

What changes have been made

Added fix to change directory logic and to cleanup pipfile's-pipenv's cache.

Verification steps

A test run in fork results in the following automated PR changes :
Successful sample test build
Generated PR - https://github.com/Srihari1192/notebooks/pull/20/files ✔️

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

Copy link
Collaborator

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

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

Thanks for the fix Abhijeet! :)

I think we shouldn't update the SDK version on any of the python3.8 files as it's no longer supported with the newer version of the SDK. Perhaps removing the python version from line 47 will suffice.

@abhijeet-dhumal abhijeet-dhumal force-pushed the fix-notebooks-sync-workflow branch from 8633ab3 to 924030b Compare June 26, 2024 09:52
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2024
Copy link
Collaborator

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

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

/lgtm

@Srihari1192
Copy link
Contributor

Srihari1192 commented Jun 26, 2024

@abhijeet-dhumal Looks like the PR generating is not updating Codeflare-sdk verison file in ubi9-python-3.9/Pipfile Srihari1192/notebooks#14

@ChristianZaccaria
Copy link
Collaborator

@Srihari1192 you're right, great catch! - We are expecting 18 file changes in this case then.

@abhijeet-dhumal abhijeet-dhumal force-pushed the fix-notebooks-sync-workflow branch from 924030b to ae7d076 Compare June 26, 2024 10:46
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2024
@abhijeet-dhumal
Copy link
Contributor Author

abhijeet-dhumal commented Jun 26, 2024

@abhijeet-dhumal Looks like the PR generating is not updating Codeflare-sdk verison file in ubi9-python-3.9/Pipfile Srihari1192/notebooks#14

Thank you so much @Srihari1192 for pointing this out ! Now it is working..
Please check : https://github.com/Srihari1192/notebooks/pull/19/files

@abhijeet-dhumal abhijeet-dhumal marked this pull request as draft June 27, 2024 06:24
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 27, 2024
@abhijeet-dhumal abhijeet-dhumal force-pushed the fix-notebooks-sync-workflow branch 2 times, most recently from d74a88a to 99d1e1c Compare June 27, 2024 13:49
@abhijeet-dhumal abhijeet-dhumal marked this pull request as ready for review June 27, 2024 14:23
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 27, 2024
@sutaakar
Copy link
Contributor

looks good to me

@abhijeet-dhumal abhijeet-dhumal force-pushed the fix-notebooks-sync-workflow branch 2 times, most recently from 1a2e84b to a68906b Compare June 28, 2024 06:40
@abhijeet-dhumal abhijeet-dhumal force-pushed the fix-notebooks-sync-workflow branch from a68906b to 75f8bf8 Compare June 28, 2024 06:57
@abhijeet-dhumal abhijeet-dhumal dismissed ChristianZaccaria’s stale review June 28, 2024 07:14

I have added some more conditional statements to consider 3.10 usecase.. It is now working seamlessly!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2024
Copy link
Collaborator

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

openshift-ci bot commented Jun 28, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ChristianZaccaria, Srihari1192
Once this PR has been reviewed and has the lgtm label, please assign astefanutti for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@abhijeet-dhumal abhijeet-dhumal merged commit 9a1d66d into project-codeflare:main Jun 28, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants