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

Create an updated style/best practices guide for kustomize packages. #1144

Merged
merged 3 commits into from
May 7, 2020

Conversation

jlewi
Copy link
Contributor

@jlewi jlewi commented Apr 30, 2020

  • See http://bit.ly/kf_kustomize_v3

  • Its been over a year since we started migrating to kustomize. In that time
    kustomize has evolved and we have learned a lot about how to use
    kustomize effectively.

  • Some of the initial usage patterns have turned out to be problematic (e.g. vars) and there are now better options.

  • Update the style guide to better document best practices.

Related to #1062

* See http://bit.ly/kf_kustomize_v3

* Its been over a year since we started migrating to kustomize. In that time
kustomize has evolved and we have learned a lot about how to use
kustomize effectively.

* Some of the initial usage patterns have turned out to be problematic (e.g. vars) and there are now better options.

* Update the style guide to better document best practices.

Related to kubeflow#1062
@kubeflow-bot
Copy link
Contributor

This change is Reviewable

@jlewi jlewi marked this pull request as ready for review April 30, 2020 02:27
@jlewi
Copy link
Contributor Author

jlewi commented Apr 30, 2020

@Bobgy @rmgogogo @yanniszark @johnugeorge @krishnadurai @yanniszark
Could you take a look at this please?

@Bobgy
Copy link
Contributor

Bobgy commented Apr 30, 2020

/lgtm
since I reviewed kustomize v3 doc

/cc @rmgogogo you may be interested in the part about var replacement

@k8s-ci-robot
Copy link
Contributor

@Bobgy: GitHub didn't allow me to request PR reviews from the following users: var, in, the, interested, part, replacement, you, may, be.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/lgtm
since I reviewed kustomize v3 doc

/cc @rmgogogo you may be interested in the part about var replacement

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Contributor

@krishnadurai krishnadurai left a comment

Choose a reason for hiding this comment

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

/lgtm


For more info see [kubeflow/manifests#1131](https://github.com/kubeflow/manifests/issues/1131)

###Resource file naming
Copy link
Member

Choose a reason for hiding this comment

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

Missing a space after ###

@terrytangyuan
Copy link
Member

@jlewi Thanks for putting this together! This is very useful. One general comment: would it be a good idea to provide an example that follows all these best practices so that we can point to specific lines from this example when illustrating each best practice.

Copy link

@Klaven Klaven left a comment

Choose a reason for hiding this comment

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

as a new person to the repo this looks really awesome! Thanks for the work you put into cleaning this up!


Currently we only have one use case which is substituting in cluster domain into virtual services ([ref](https://docs.google.com/document/d/1jBayuR5YvhuGcIVAgB1F_q4NrlzUryZPyI9lCdkFTcw/edit#heading=h.vyq4iltpirga)).

We would ultimately like to get rid of the use of vars in these cases but have not settled on precise solutions. Some possible options are
Copy link

Choose a reason for hiding this comment

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

IMO stating that you should not use global substitution except where there is no other option is enough in the best practices, no need to delve into possible solutions. Maybe just make a overarching issue where discussion can be had and just link there "if you want more info"?

Its fine as is. just my preference.

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 think I'd like to nudge people towards using kpt setters. I guess we could just as easily link to an issue with recommended patterns and describe uses there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Klaven this is a good suggestion. I thought about following your suggestion and just linking to the bug #1007. The issue is getting quite long though so then I thought it would be valuable to kind of summarize it here. I also want to nudge people to start trying/using kpt setters.

@yanniszark
Copy link
Contributor

@jlewi thanks a lot for putting this together!
Some thoughts are:

  • In the design doc, the --load_restrictor none option is mentioned in order to reuse patches. Should we mention that we allow its use and encourage reusing patches in this doc?
  • Should we mention the global Kubeflow configmap for common options (e.g., kubeflow-userid)?

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 4, 2020
@jlewi
Copy link
Contributor Author

jlewi commented May 4, 2020

@yanniszark

In the design doc, the --load_restrictor none option is mentioned in order to reuse patches. Should we mention that we allow its use and encourage reusing patches in this doc?

Done.

Should we mention the global Kubeflow configmap for common options (e.g., kubeflow-userid)?

I'd forgotten about that. The global configmap seems like an inferior solution to kpt setters. So I think I'd rather nudge people to kpt setters.

The problem with the global configmap is that we then need to have variations of the application package which use the global config map as opposed to the application configmap. With kpt setters we should be able to kustomize package complete on its own while also allowing global parameters to be set once.

@jlewi
Copy link
Contributor Author

jlewi commented May 7, 2020

@yanniszark @krishnadurai could one of you LGTM please?

@krishnadurai
Copy link
Contributor

/lgtm

@jlewi
Copy link
Contributor Author

jlewi commented May 7, 2020

Thanks

/approve

@k8s-ci-robot
Copy link
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

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

@k8s-ci-robot k8s-ci-robot merged commit 346db1f into kubeflow:master May 7, 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.

9 participants