Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore: update project structure #86

Merged
merged 8 commits into from
Jun 27, 2023
Merged

chore: update project structure #86

merged 8 commits into from
Jun 27, 2023

Conversation

ramizpolic
Copy link
Member

@ramizpolic ramizpolic commented Jun 23, 2023

Overview

Resolves #44

Notes for reviewer

Uses new paths for various resources, given as:

├── deploy
│   ├── charts - defines helm chart resources
│   ├── defaults - defines resources required to run examples/operator tests
│   ├── dev - defines various development env deployment scripts
│   │   ├── microk8s - example deployments for microk8s env
│   │   └── multi-dc - deployment scripts for running vault instances across multiple clusters for local and AWS envs
│   └── examples - defines example resources for deploying operator
├── hack
│   └── scripts

Also, all custom resources in deploy/examples dir have been moved to independent files which define complete examples.

@sagikazarmark
Copy link
Member

Couple notes:

  • I'd much rather split this up into smaller PRs
  • deploy/crd is not quite right: those are rather CR examples
  • deploy/test move doesn't feel right, especially since it's gonna be rewritten at some point in the style of the vault-secrets-webhook e2e tests
  • Can't find deploy/files anywhere yet there are references to it in code

@ramizpolic
Copy link
Member Author

ramizpolic commented Jun 26, 2023

In that case, I'd recommend using config directory (similar to how kubebuilder does CRD generation) which can be a common parent to all the mentioned resources.
I could split into multiple PRs, but I feel it won't really matter as much since only a couple of tree changes are impactful.

@sagikazarmark
Copy link
Member

In that case, I'd recommend using config directory (similar to how kubebuilder does CRD generation) which can be a common parent to all the mentioned resources.

That doesn't feel quite right either. I mean, not for all (eg. tests).

Here is my recommendation:

  • deploy/charts/...
  • deploy/examples -> CR examples
  • deploy/examples/base (?) -> base resources (rbac) for examples
  • deploy/examples/... -> additional directories for examples with multiple resources (alternatively: put different objects into one yaml file for these examples)

@ramizpolic
Copy link
Member Author

In that case, I'd recommend using config directory (similar to how kubebuilder does CRD generation) which can be a common parent to all the mentioned resources.

That doesn't feel quite right either. I mean, not for all (eg. tests).

Here is my recommendation:

  • deploy/charts/...
  • deploy/examples -> CR examples
  • deploy/examples/base (?) -> base resources (rbac) for examples
  • deploy/examples/... -> additional directories for examples with multiple resources (alternatively: put different objects into one yaml file for these examples)

Adding to this, if we were to put scripts to hack/scripts, we could also then move tests to hack/tests, which would fit more clearly. I wanted to avoid deploy dir since it is used as a transient dir during resource generation.

@sagikazarmark
Copy link
Member

Adding to this, if we were to put scripts to hack/scripts

Sounds good to me

we could also then move tests to hack/tests

Let's not touch tests for now. As I said, I wanna move them to e2e (which is another loosly applied pattern out there) once we rewrite them using the e2e framework.

I wanted to avoid deploy dir since it is used as a transient dir during resource generation.

deploy is a widely used pattern IMO. I generally prefer it over config.

@ramizpolic ramizpolic closed this Jun 26, 2023
@ramizpolic ramizpolic reopened this Jun 26, 2023
@ramizpolic ramizpolic changed the title chore: update project params and structure chore: update project deploy structure Jun 26, 2023
@ramizpolic ramizpolic changed the title chore: update project deploy structure chore: update project structure Jun 26, 2023
@ramizpolic ramizpolic self-assigned this Jun 26, 2023
akijakya
akijakya previously approved these changes Jun 27, 2023
Copy link
Member

@akijakya akijakya left a comment

Choose a reason for hiding this comment

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

I left some comments but otherwise LGTM 💯

deploy/examples/cr-istio.yaml Outdated Show resolved Hide resolved
deploy/examples/cr-istio.yaml Outdated Show resolved Hide resolved
akijakya
akijakya previously approved these changes Jun 27, 2023
Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Couple more nits. Other than those, LGTM.

.dockerignore Outdated Show resolved Hide resolved
hack/scripts/validate-config-crud/validate-config-crud.sh Outdated Show resolved Hide resolved
.yamlignore Outdated Show resolved Hide resolved
Signed-off-by: Ramiz Polic <ramiz.polic@hotmail.com>
Signed-off-by: Ramiz Polic <ramiz.polic@hotmail.com>
Signed-off-by: Ramiz Polic <ramiz.polic@hotmail.com>
Signed-off-by: Ramiz Polic <ramiz.polic@hotmail.com>
Signed-off-by: Ramiz Polic <ramiz.polic@hotmail.com>
Signed-off-by: Ramiz Polic <ramiz.polic@hotmail.com>
Signed-off-by: Ramiz Polic <ramiz.polic@hotmail.com>
Signed-off-by: Ramiz Polic <ramiz.polic@hotmail.com>
@ramizpolic ramizpolic requested a review from akijakya June 27, 2023 15:51
Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

LGTM thanks @ramizpolic

Nice work!

@ramizpolic ramizpolic merged commit 48c5360 into main Jun 27, 2023
@ramizpolic ramizpolic deleted the chore/repo-update branch June 27, 2023 16:36
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reorganize deployment resources
3 participants