Skip to content
This repository has been archived by the owner on Nov 29, 2023. It is now read-only.

111 create a configure cluster command #113

Merged
merged 7 commits into from Jun 30, 2021

Conversation

Marios85
Copy link
Contributor

This PR closes #111. Tweaked the existing code for setting up a namespace so the job-deployment accounts have permission to create namespaces. This in turn is called by the new command to configure the cluster.

Copy link
Contributor

@AlexIoannides AlexIoannides left a comment

Choose a reason for hiding this comment

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

A couple of points - happy to jump on a call to discuss.

src/bodywork/cli/cli.py Show resolved Hide resolved
src/bodywork/constants.py Outdated Show resolved Hide resolved
src/bodywork/k8s/auth.py Outdated Show resolved Hide resolved
tests/unit_and_functional/test_cli.py Show resolved Hide resolved
Copy link
Contributor

@AlexIoannides AlexIoannides left a comment

Choose a reason for hiding this comment

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

I think the logic is good-to-go - we just need to get the naming sorted so that it makes sense.

We have two sets of entity that required service-accounts:

  • pods running the workflow-controller (i.e. in a 'deployment job'); and,
  • pods that form the stages of a workflow (be them jobs or k8s-deployment resources).

Prior to this PR, we have BODYWORK_WORKFLOW_SERVICE_ACCOUNT for the first entity and BODYWORK_JOBS_DEPLOYMENTS_SERVICE_ACCOUNT for the second.

We need to pick names that done't confuse in the future.

src/bodywork/constants.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 29, 2021

Codecov Report

❗ No coverage uploaded for pull request base (v3.0.0@2f2a0b7). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             v3.0.0     #113   +/-   ##
=========================================
  Coverage          ?   97.90%           
=========================================
  Files             ?       21           
  Lines             ?     1383           
  Branches          ?        0           
=========================================
  Hits              ?     1354           
  Misses            ?       29           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f2a0b7...20a1c0a. Read the comment docs.

@Marios85
Copy link
Contributor Author

Renaming has been done and added a test fixture that configures the cluster for the tests that require it.

@AlexIoannides
Copy link
Contributor

AlexIoannides commented Jun 29, 2021

setup_workflow_service_account(...) needs a little modification, so that the workflow-controller is authorised to delete namespaces, as well.

Copy link
Contributor

@AlexIoannides AlexIoannides left a comment

Choose a reason for hiding this comment

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

Just a couple of minor point and then good to go - doesn't need another review.

@Marios85
Copy link
Contributor Author

I have made the changes.

I didn't add the delete command as I thought that the user would be issuing the command to delete a namespace but then I suppose the workflow controller might want to tear down the namespace on fail etc.

@Marios85 Marios85 merged commit 4f6755d into v3.0.0 Jun 30, 2021
@AlexIoannides AlexIoannides deleted the 111-Create-a-configure-cluster-command branch June 30, 2021 07:22
@AlexIoannides
Copy link
Contributor

Yes, that's right - if a deployment doesn't leave any services hanging around (i.e. just batch jobs), we should probably delete the namespace, too.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants