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

Step 1: Reorganize folder structure #1739

Merged

Conversation

yanniszark
Copy link
Contributor

@yanniszark yanniszark commented Feb 8, 2021

Which issue is resolved by this Pull Request:
Resolves #1738
Umbrella issue: #1735

Description of your changes:
Currently, the manifests repo is a flat namespace with everything mangled.
This PR reorganizes the folder structure into sections that will reflect the
following properties:

  • Applications with their source code in upstream KF maintained by Kubeflow WGs (e.g., notebook-controller)
  • Common services (Dex, Istio, Cert-Manager, KNative), maintained by the
    Manifests WG.
  • Applications contributed by community members, not owned by any Kubeflow WG.
  • Distribution-specific manifests (kfdef, stacks, aws, gcp, etc.). The goal is
    for this folder to become empty in subsequent releases.

@yanniszark yanniszark changed the title Feature step1 folder restructuring Reorganize folder structure Feb 8, 2021
@google-cla google-cla bot added the cla: yes label Feb 8, 2021
@yanniszark
Copy link
Contributor Author

/cc @kubeflow/wg-manifests-leads

@k8s-ci-robot k8s-ci-robot requested a review from a team February 8, 2021 17:25
@yanniszark yanniszark changed the title Reorganize folder structure Step 1: Reorganize folder structure Feb 8, 2021
@yanniszark yanniszark force-pushed the feature-step1-folder-restructuring branch from f7d4101 to 4e19e6f Compare February 9, 2021 15:06
@elikatsis
Copy link
Member

Thank you Yannis!

I'll review this
/assign @elikatsis

…eam'

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
…upstream'

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
…eb-app/upstream'

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
…ook-controller/upstream'

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Update the README to reflect the new folder structure of manifests. In
addition, move the obsolete information about kfctl manifests into its
own doc.

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Update the unit tests for the new manifests structure. Temporarily
disable the autogenerated tests on stacks. Consider re-enabling them
before the 1.3 release. In addition, evaluate if we need the legacy
kustomizations tests. Perhaps a simpler sanity test of building
all kustomizations in the repo would be preferrable.

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
@yanniszark yanniszark force-pushed the feature-step1-folder-restructuring branch from ce4b28a to 5f4adf1 Compare February 9, 2021 22:23
@moficodes
Copy link
Contributor

@yanniszark are we updating the paths for the kfdefs?

would it not break the kfdef at this state?

@yanniszark
Copy link
Contributor Author

are we updating the paths for the kfdefs?

@moficodes not at the moment. This is something planned after step 4 (#1735) because the structure of many upstream kustomizations may change again. Before the 1.3 release, distribution owners will have enough time to import the kustomizations they want.

would it not break the kfdef at this state?

Correct! However, interim commits are not guarranteed to be good, especially in the middle of this restructuring. In contrast, the release will be validated.

@moficodes
Copy link
Contributor

@yanniszark thanks for the clarification.

Just to confirm, are we merging all 4 phases at once? becase there are kfdefs that rely on master. and if we merge this to master those kfdefs stop working.

@PatrickXYS
Copy link
Member

@yanniszark thanks for the clarification.

Just to confirm, are we merging all 4 phases at once? becase there are kfdefs that rely on master. and if we merge this to master those kfdefs stop working.

I think we need to rely on stable branches instead, e.g, v1.1-branch, v1.2-brannch, the user should use kfdef referenced in corresponding branches instead.

Can you clarify what's the use case user need to use kfdef in the master branch in production environment?

@moficodes
Copy link
Contributor

for example the kfdef for openshift that uses tekton for pipeline is using master. (i think its the only kfdef like this) https://github.com/kubeflow/manifests/blob/master/kfdef/kfctl_openshift.master.kfptekton.yaml

i think its fine since master is a development branch.

@PatrickXYS
Copy link
Member

I see it's because this is done after 1.2,

but we haven't cut 1.3 release yet

@moficodes
Copy link
Contributor

Yeah. I dont think its going to break any production application. But if we merge something to master I would think we would want make sure all the kfdef targetting master is working.

@yanniszark
Copy link
Contributor Author

@moficodes manifests only provides a guarantee for release branches to be stable. It is expected that some changes (like paths changes) can break distributions between releases. Distributions should be updated asynchronously, after the release of the stable branch.
After all, in the near future, where distributions will live outside manifests, it won't be reasonable to block manifests PRs on them. This is the same case now.

@moficodes
Copy link
Contributor

Understood. Thanks @yanniszark

@elikatsis
Copy link
Member

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elikatsis, yanniszark

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:
  • OWNERS [elikatsis,yanniszark]

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

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.

Step 1: Reorganize folder structure
5 participants