Skip to content

Add Katib CRD to components#1220

Merged
k8s-ci-robot merged 2 commits into
kubeflow:masterfrom
andreyvelich:katib-crd-to-components
Jun 3, 2020
Merged

Add Katib CRD to components#1220
k8s-ci-robot merged 2 commits into
kubeflow:masterfrom
andreyvelich:katib-crd-to-components

Conversation

@andreyvelich

@andreyvelich andreyvelich commented Jun 1, 2020

Copy link
Copy Markdown
Member

I added Katib CRDs to components, see discussion: #1124 (comment).

Also, I added commonLabels to components/katib-db-manager and components/katib-db-mysql and I removed commonLabels from installs/katib-standalone since it is not required.

@jlewi What do you think about Katib manifests v3 version? katib/installs and katib/components satisfies the V3 manifests rules.
I can delete current v3 manifests and make installs and components under v3 folder.

/assign @jlewi @gaocegege @johnugeorge

Checklist:

  • Unit tests have been rebuilt:
    1. cd manifests/tests
    2. make generate-changed-only
    3. make test

@kubeflow-bot

Copy link
Copy Markdown
Contributor

This change is Reviewable

@gaocegege gaocegege left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

I am not familiar with kustomize, thus prefer another review from kustomize experts.

newName: mysql
commonLabels:
app.kubernetes.io/component: katib
app.kubernetes.io/name: katib-controller

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this the right name? Did you mean to call everything katib-controller even if its a db?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Currently, it works like this.
We have one Application: https://github.com/kubeflow/manifests/blob/master/katib/katib-controller/overlays/application/application.yaml, for katib-controller, katib-db-manager and katib-mysql with this label (https://github.com/kubeflow/manifests/blob/master/katib/katib-controller/overlays/application/kustomization.yaml#L4).
If we want to make unique label for each component, do we need to create individual application for them?

@jlewi

jlewi commented Jun 2, 2020

Copy link
Copy Markdown
Contributor

I can delete current v3 manifests and make installs and components under v3 folder

I didn't quite understand this. What are you proposing?

@andreyvelich

Copy link
Copy Markdown
Member Author

I can delete current v3 manifests and make installs and components under v3 folder

I didn't quite understand this. What are you proposing?

I mean, we created Katib V3 manifest for katib-controller folder (https://github.com/kubeflow/manifests/tree/master/katib/katib-controller/v3) and katib-crds folder (https://github.com/kubeflow/manifests/tree/master/katib/katib-crds/v3).

After we migrated to folder structure katib/components and katib/installs these folders (katib/katib-controller and katib/katib-crds) are legacy (but we still use them in kfdef manifests).

In the new V3 version, I think we can use this new structure with components and installs for Katib.
So we can remove V3 version that related to old folders.

And I am asking do we need to point that new folder structure is related to V3 or it is not necessary?

@jlewi

jlewi commented Jun 3, 2020

Copy link
Copy Markdown
Contributor

In the new V3 version, I think we can use this new structure with components and installs for Katib.
So we can remove V3 version that related to old folders.
Sounds good to me.

And I am asking do we need to point that new folder structure is related to V3 or it is not necessary?

I may not have understood what you are saying here. I think using the new folder structure is fine. You don't need to call it V3 .

/lgtm
/approve

/hold
In case you want to do the additional cleanup in this PR; otherwise you can remove the hold and submit as is.

@k8s-ci-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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

The pull request process is described here

Details 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

@andreyvelich

Copy link
Copy Markdown
Member Author

In the new V3 version, I think we can use this new structure with components and installs for Katib.
So we can remove V3 version that related to old folders.
Sounds good to me.

And I am asking do we need to point that new folder structure is related to V3 or it is not necessary?

I may not have understood what you are saying here. I think using the new folder structure is fine. You don't need to call it V3 .

/lgtm
/approve

/hold
In case you want to do the additional cleanup in this PR; otherwise you can remove the hold and submit as is.

Thanks! I will submit another PR to remove old v3 folders.

@andreyvelich

Copy link
Copy Markdown
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit e017e2b into kubeflow:master Jun 3, 2020
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.

7 participants