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

Update Katib operator and image #1465

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

knkski
Copy link
Contributor

@knkski knkski commented Mar 10, 2021

What this PR does / why we need it:

Updates Docker image to include changes from #1450, and updates operator to latest version of operator framework.

Which issue(s) this PR fixes:
Fixes #1453

Copy link
Contributor

@DomFleischmann DomFleischmann left a comment

Choose a reason for hiding this comment

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

Minor naming nitpick, everything else LGTM

operators/katib-db-manager/src/charm.py Outdated Show resolved Hide resolved
Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @knkski!
I left few comments.

operators/katib-controller/src/charm.py Show resolved Hide resolved

validating, mutating = yaml.safe_load_all(Path("src/webhooks.yaml").read_text())

self.model.pod.set_spec(
Copy link
Member

@andreyvelich andreyvelich Mar 11, 2021

Choose a reason for hiding this comment

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

I think it would be great to create a script to automatically generate this spec from the appropriate YAML manifests.

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've opened this issue to talk about supporting it in the operator framework directly:

canonical/operator#484

@@ -0,0 +1,77 @@
apiVersion: admissionregistration.k8s.io/v1
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to have your own YAML for the webhooks or you can use it from the source: /manifest/v1beta1/... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See canonical/charmcraft#260 for why we can't symlink it to the existing file. If you'd like, I could take a look at adding a Makefile or similar that would copy the file over before calling charmcraft build.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thank you for the information.
Maybe we can include this file copy in the script that I mentioned here: #1465 (comment).

Thus, this script will take care about changes between operator manifests and the Katib original manifests.
WDYT @knkski ?

We can add this script in the future PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, sounds good 👍

@knkski knkski force-pushed the katib-operator-updates branch 2 times, most recently from e1d8648 to c9de12d Compare March 11, 2021 17:22
Updates Docker image to include changes from kubeflow#1450, and updates
operator to latest version of operator framework.
@andreyvelich
Copy link
Member

andreyvelich commented Mar 12, 2021

@knkski Should we merge this PR ?
I will create an issue for the comment mentioned in this thread: #1465 (comment)

@knkski
Copy link
Contributor Author

knkski commented Mar 12, 2021

@andreyvelich: Yeah, this should be good to merge

@andreyvelich
Copy link
Member

Thank you for doing this @knkski!
/lgtm
/approve

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, DomFleischmann, knkski

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

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, DomFleischmann, knkski

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

@google-oss-robot google-oss-robot merged commit badbbdb into kubeflow:master Mar 12, 2021
andreyvelich pushed a commit to andreyvelich/katib that referenced this pull request Mar 12, 2021
Updates Docker image to include changes from kubeflow#1450, and updates
operator to latest version of operator framework.
andreyvelich pushed a commit to andreyvelich/katib that referenced this pull request Mar 12, 2021
Updates Docker image to include changes from kubeflow#1450, and updates
operator to latest version of operator framework.
andreyvelich pushed a commit to andreyvelich/katib that referenced this pull request Mar 12, 2021
Updates Docker image to include changes from kubeflow#1450, and updates
operator to latest version of operator framework.
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.

[Juju] Update Juju operator with webhook manifests changes
4 participants