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

Support providing serviceAccountName with create/update/apply commands #76

Merged
merged 2 commits into from
Apr 20, 2022

Conversation

warango4
Copy link
Contributor

Pull request

What this PR does / why we need it

Add support to se serviceAccountName property in a workload yaml file.
It can be added while created or added after creation in yaml file and using update/apply commands for the workload to change

Which issue(s) this PR fixes

Fixes #64

Describe testing done for PR

  • Unit testing inside source code.
  • Created a workload with an example yaml file with the property set.
  • Created a workload with an example yaml file without the property, then changed it to add it as part of the spec, used both, update and apply commands to check if the change was taken.

Additional information or special notes for your reviewer

Signed-off by: Wendy Arango warango@vmware.com

@warango4
Copy link
Contributor Author

Create a workload with serviceAccountName in the spec section

image

@warango4
Copy link
Contributor Author

Delete serviceAccountName from spec section

image

@warango4
Copy link
Contributor Author

Change current serviceAccountName using apply

image

@warango4 warango4 force-pushed the issue64 branch 2 times, most recently from 6e6e508 to 85a3b71 Compare April 19, 2022 16:49
…ommands

serviceAccountName, as part of spec in workload, can be now set in a yaml file, as well as deleted.
Just adding something like the following in the workload yaml file, this specification will be taken by the supply chain

---
apiVersion: carto.run/v1alpha1
kind: Workload
metadata: {}
spec:
...
  serviceAccountName: <string>
...

If this property doesn't belong to the workload anymore, it can be deleted by simply removing it from the yaml file and then using update/apply commands.

Signed-off by: Wendy Arango warango@vmware.com
Copy link
Contributor

@odinnordico odinnordico left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/apis/cartographer/v1alpha1/workload_helpers.go Outdated Show resolved Hide resolved
Copy link
Member

@rashedkvm rashedkvm left a comment

Choose a reason for hiding this comment

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

Please see comment here. Add test shows removing spec.serviceAccountName as required by comment

Copy link
Member

@rashedkvm rashedkvm left a comment

Choose a reason for hiding this comment

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

for workload create/update/apply In the supplied yaml has serviceAccountName="", removes the serviceAccountName field from workload.

@warango4
Copy link
Contributor Author

Deleting serviceAccountName by setting it to serviceAccountName=""

image

@rashedkvm rashedkvm merged commit fc7b718 into vmware-tanzu:main Apr 20, 2022
@warango4 warango4 deleted the issue64 branch April 20, 2022 16:14
@warango4 warango4 restored the issue64 branch April 20, 2022 19:25
@heyjcollins
Copy link
Contributor

heyjcollins commented Apr 21, 2022

Acceptance

Looks good overall however...

if I execute any update that doesn't include
spec.serviceAccountName key/value explicitly in the incantation, spec.serviceAccountName the plugin removes it from the workload object.
The plugin should only remove it when spec.serviceAccountName: "" is provided:
Screen Shot 2022-04-20 at 7 00 02 PM

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.

workload create/update/apply should support providing the spec.ServiceAccountName via workload.yaml
5 participants