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

automation of the hash update on the annotation with digest updater #375

Merged
merged 1 commit into from
Dec 8, 2023
Merged

automation of the hash update on the annotation with digest updater #375

merged 1 commit into from
Dec 8, 2023

Conversation

dibryant
Copy link
Contributor

fixes for #354

Description

I followed each step as described here.

How Has This Been Tested?

I tested with kustomize build manifests/base/.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

harshad16
harshad16 requested changes Nov 29, 2023
View reviewed changes
Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

Great effort
Shared few suggestions.

manifests/base/kustomization.yaml Outdated Show resolved Hide resolved
manifests/base/kustomization.yaml Outdated Show resolved Hide resolved
manifests/base/kustomization.yaml Outdated Show resolved Hide resolved
manifests/base/kustomization.yaml Show resolved Hide resolved
.github/workflows/notebooks-digest-updater-upstream.yaml Outdated Show resolved Hide resolved
.github/workflows/notebooks-digest-updater-upstream.yaml Show resolved Hide resolved
@dibryant dibryant force-pushed the digest_updater branch 2 times, most recently from 5a1a374 to 5a3c785 Compare November 29, 2023 16:13
atheo89
atheo89 requested changes Nov 30, 2023
View reviewed changes
Copy link
Member

@atheo89 atheo89 left a comment

Choose a reason for hiding this comment

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

Made a minor suggestion; overall, it looks good to me! I've tested the recent integration in my local repository, and it's functioning correctly. The commit.env file is being updated successfully. However, I haven't tested whether the kustomization.yaml file captures the new ConfigMap and subsequently updates the ImageStreams. Could you please ensure that it working well on the cluster side? You can use the instructions from this link: https://github.com/opendatahub-io/opendatahub-operator/wiki/3.-Using-Operator-in-Dev-Mode (The last example fits for the workbences)

manifests/base/jupyter-minimal-gpu-notebook-imagestream.yaml Outdated Show resolved Hide resolved
harshad16
harshad16 requested changes Dec 6, 2023
View reviewed changes
Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

Still Reviewing this PR
Please address the following suggestions.

manifests/base/commit.yaml Outdated Show resolved Hide resolved
manifests/base/commit.yaml Outdated Show resolved Hide resolved
@dibryant dibryant force-pushed the digest_updater branch 4 times, most recently from 43d99bf to f5434cf Compare December 8, 2023 16:06
harshad16
harshad16 reviewed Dec 8, 2023
View reviewed changes
Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

lgtm
just one more change needed:

manifests/base/jupyter-pytorch-notebook-imagestream.yaml Outdated Show resolved Hide resolved
harshad16
harshad16 approved these changes Dec 8, 2023
View reviewed changes
Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

thanks for the work.

@openshift-ci openshift-ci bot added the lgtm label Dec 8, 2023
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 8, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshad16

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved label Dec 8, 2023
@harshad16 harshad16 merged commit cadbe9e into opendatahub-io:main Dec 8, 2023
0 of 5 checks passed
@harshad16 harshad16 mentioned this pull request Dec 8, 2023
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atheo89 atheo89 atheo89 requested changes

@harshad16 harshad16 harshad16 approved these changes

@rkpattnaik780 rkpattnaik780 Awaiting requested review from rkpattnaik780

Assignees

@harshad16 harshad16

@atheo89 atheo89

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dibryant @harshad16 @atheo89 @openshift-merge-robot