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

476: change-rule to delete packageInstall/app before service account and RBAC #552

Merged
merged 12 commits into from
Jul 21, 2022

Conversation

kumaritanushree
Copy link
Contributor

@kumaritanushree kumaritanushree commented Jul 8, 2022

What this PR does / why we need it:

This PR has change-rule added in default.go to delete packageInstall/app (kctrl App CR) before service account and RBAC

Which issue(s) this PR fixes:

Fixes #476

Does this PR introduce a user-facing change?

NONE

Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:


@kumaritanushree
Copy link
Contributor Author

kumaritanushree commented Jul 8, 2022

With these changes ordering of upsert and deletion of resources packageInstall/App, service account and RBAC will be:

Upsert: (There is no change in upsert rule, already working as per requirement with the existing rules)

Changes are sorted in order that they will be applied. They will be applied in parallel within their group. Each change lists other changes that it will wait for before being applied.

1. 
     1. (upsert) serviceaccount/default-ns-sa (v1) namespace: default (0)
     2. (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default (0)
     3. (upsert) secret/pkg-demo-values (v1) namespace: default (0)
2.
     1. (upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default (+1) ^
3.
     1. (upsert) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default (+4) ^
     2. (upsert) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) namespace: default (+4) ^

Delete:

Changes are sorted in order that they will be applied. They will be applied in parallel within their group. Each change lists other changes that it will wait for before being applied.

1. 
    1. (delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) namespace: default (0)
    2. (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default (0)
    3. (delete) secret/pkg-demo-values (v1) namespace: default (0)
2.
    1. (delete) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default (+1) ^
    2. (delete) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default (+1) ^
    3. (delete) serviceaccount/default-ns-sa (v1) namespace: default (+1) ^        4. 

@kumaritanushree kumaritanushree marked this pull request as ready for review July 12, 2022 09:13
pkg/kapp/diffgraph/assets/appcr-rbac-svcaccnt.yaml Outdated Show resolved Hide resolved
pkg/kapp/diffgraph/change_graph_test.go Outdated Show resolved Hide resolved
pkg/kapp/diffgraph/change_graph_test.go Outdated Show resolved Hide resolved
pkg/kapp/diffgraph/change_graph_test.go Outdated Show resolved Hide resolved
pkg/kapp/config/default.go Outdated Show resolved Hide resolved
pkg/kapp/diffgraph/change_graph_test.go Outdated Show resolved Hide resolved
pkg/kapp/diffgraph/change_graph_test.go Outdated Show resolved Hide resolved
pkg/kapp/diffgraph/change_graph_test.go Outdated Show resolved Hide resolved
pkg/kapp/config/default.go Outdated Show resolved Hide resolved
pkg/kapp/diffgraph/change_graph_test.go Outdated Show resolved Hide resolved
pkg/kapp/config/default.go Outdated Show resolved Hide resolved
pkg/kapp/config/default.go Outdated Show resolved Hide resolved
pkg/kapp/diffgraph/change_graph_test.go Outdated Show resolved Hide resolved
Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

Looking pretty good now.

pkg/kapp/diffgraph/change_graph_test.go Outdated Show resolved Hide resolved
pkg/kapp/diffgraph/change_graph_test.go Outdated Show resolved Hide resolved
resourceMatchers:
- apiVersionKindMatcher: {kind: App, apiVersion: kappctrl.k14s.io/v1alpha1}

- name: change-groups.kapp.k14s.io/packageinstall
Copy link
Contributor

Choose a reason for hiding this comment

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

change-groups.kapp.k14s.io/packageinstall => change-groups.kapp.k14s.io/kapp-controller-packageinstall

Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

LGTM!

@cppforlife cppforlife merged commit 5a2eb44 into develop Jul 21, 2022
@cppforlife cppforlife deleted the task/476-apply-order-rbac branch July 21, 2022 15:44
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.

augment default rules with ordering for App/PackageInstall
5 participants