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

scheduler: allow to specify namespace #77

Closed
wants to merge 1 commit into from
Closed

scheduler: allow to specify namespace #77

wants to merge 1 commit into from

Conversation

Tal-or
Copy link
Contributor

@Tal-or Tal-or commented Dec 16, 2021

This change is good in general, but also will help with implementing the scheduler deployment in NROP.
The change is completely optional and won't affect the default behavior

Signed-off-by: Talor Itzhak titzhak@redhat.com

@Tal-or Tal-or requested a review from ffromani December 16, 2021 08:18
@Tal-or
Copy link
Contributor Author

Tal-or commented Dec 16, 2021

/hold

@ffromani
Copy link
Collaborator

I don't see the benefit for the deployer here, could you please elaborate?

@Tal-or
Copy link
Contributor Author

Tal-or commented Dec 16, 2021

I don't see the benefit for the deployer here, could you please elaborate?

It'll allow selecting the namespace where you want the scheduler to get deployed in the deployer as well

@ffromani
Copy link
Collaborator

I don't see the benefit for the deployer here, could you please elaborate?

It'll allow to select the namespace where you want the scheduler to get deployed

but in the deployer we do want to have opinionated defaults. Selecting the namespace will create more complications and we don't have a usecase for that yet.

So maybe let's start with the use case in the contex to of the deployer, so we can evaluate the best solution.

This change is good in general, but also will help with implementing the scheduler deployment in NROP.
The change is completely optional and won't affect the default behavior

Signed-off-by: Talor Itzhak <titzhak@redhat.com>
@Tal-or
Copy link
Contributor Author

Tal-or commented Dec 16, 2021

but in the deployer we do want to have opinionated defaults.

And we do, in case that the namespace is not provided we're staying with the default.
Changing the namespace is totally optional

So maybe let's start with the use case in the contex to of the deployer, so we can evaluate the best solution.

I mentioned the context in the PR, but I can elaborate more if needed

Anyhow, I also have a workaround in case we don't want to have this patch in the deployer (it's just that the workaround is a bit ugly and has some copy-paste from the original deployer code)

@ffromani
Copy link
Collaborator

but in the deployer we do want to have opinionated defaults.

And we do, in case that the namespace is not provided we're staying with the default. Changing the namespace is totally optional

True, but then we need to support (and test) this option, whose benefit is still not clear to me

@ffromani ffromani closed this Dec 16, 2021
@ffromani
Copy link
Collaborator

wrong button

@ffromani ffromani reopened this Dec 16, 2021
@Tal-or
Copy link
Contributor Author

Tal-or commented Dec 16, 2021

True, but then we need to support (and test) this option, whose benefit is still not clear to me

OK let's leave it for now, we can discuss this PR again once I'll have the changes in NROP ready.
Then I believe the benefit will be more clear.

@Tal-or Tal-or closed this Dec 16, 2021
@ffromani ffromani deleted the ns_opt branch December 29, 2022 08:28
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.

None yet

2 participants