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

Custom COU Namespace #221

Merged
merged 4 commits into from
Jan 18, 2024

Conversation

gabrielcocenza
Copy link
Member

  • create a new NamedTuple class to be the COU Namespace. With that, type is still checked and few arguments are necessary to pass to the functions that use the arguments passed by the user. Moreover, docstrings are shorter and easier to maintain

  • Global variables for "data-plane" and "control-plane".

- create a new NamedTuple class to be the COU Namespace. With that,
  type is still checked and few arguments are necessary to pass to
  the functions that use the arguments passed by the user. Moreover,
  docstrings are shorter and easier to maintain

- Global variables for "data-plane" and "control-plane".
Copy link
Contributor

@rgildein rgildein left a comment

Choose a reason for hiding this comment

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

I will suggest renaming Namespace to something else and maybe add prompt as property or additional argument set during init process.
I see some benefits passing CLI args to everywhere, so I will give +1.

cou/commands.py Outdated Show resolved Hide resolved
cou/cli.py Show resolved Hide resolved
cou/cli.py Outdated Show resolved Hide resolved
cou/commands.py Outdated Show resolved Hide resolved
Copy link
Contributor

@agileshaw agileshaw left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just one comment inline.

cou/cli.py Show resolved Hide resolved
Copy link
Contributor

@rgildein rgildein left a comment

Choose a reason for hiding this comment

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

I approve, but I still don't see it as a big benefit.

cou/cli.py Show resolved Hide resolved
cou/commands.py Outdated Show resolved Hide resolved
Copy link
Contributor

@agileshaw agileshaw left a comment

Choose a reason for hiding this comment

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

I see the benefit of having less arguments passed into functions, but I think we are hyper-correcting it a bit with this PR and I have concerns about what this extra layer of abstraction may bring. But this is not a blocker, so I'm also approving it.

@gabrielcocenza gabrielcocenza merged commit 8add88f into canonical:dev/data-plane Jan 18, 2024
1 check passed
rgildein pushed a commit that referenced this pull request Jan 24, 2024
- check if control-plane is already upgraded if user pass "data-pane"
upgrade group
- check if data-plane apps exists if user pass "data-pane" upgrade group
- splitted the logic on determine_upgrade_target to several functions

This PR should be merged after #221
@gabrielcocenza gabrielcocenza deleted the cou_namespace branch March 12, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants