Add option for budget approval ticket

[smalleni]

Description

Adds an optional parameter for tagging resources created in AWS with cloud spend approval ticket id.

Fixes

[dry923]

IMHO this and #157 should go to simply a generic key:value list option exactly like it is to the rosa cli. Its then a simple append to the rosa create command.
Something to the extent of:
--rosa-tags "foo:bar,bar:foobar"

Thoughts @smalleni @morenod @krishvoor ?

[morenod]

What I think is that this new parameter --ticket-id makes sense if we change it to required=True, and we validate somehow that it is a valid ticket. if it is going to be optional and directly passed to the create command, we should use the existing --wildcard-options one

What we decided about the parameters when we started to work on this wrapper was that we will only add parameters that we need to use, manage, transform or validate in the wrapper. Any parameter that is going to be directly passed to the rosa create command can be added in the --wildcard-options parameter.

For example, we dont have any parameter for the version of openshift to be installed, because we are not validating it on any way, so if you want to specify a version to install, you can use, for example --wildcard-options "--channel-group candidate --version 4.13.0-rc.4"

This is the same scenario, the only scope for this parameter is to pass across all the functions to arrive the cluster create command, and we can do that with any code change as --wildcard-options "--tags=TicketId:XXXX

[dry923]

What I think is that this new parameter --ticket-id makes sense if we change it to required=True, and we validate somehow that it is a valid ticket. if it is going to be optional and directly passed to the create command, we should use the existing --wildcard-options one

What we decided about the parameters when we started to work on this wrapper was that we will only add parameters that we need to use, manage, transform or validate in the wrapper. Any parameter that is going to be directly passed to the rosa create command can be added in the --wildcard-options parameter.

For example, we dont have any parameter for the version of openshift to be installed, because we are not validating it on any way, so if you want to specify a version to install, you can use, for example --wildcard-options "--channel-group candidate --version 4.13.0-rc.4"

This is the same scenario, the only scope for this parameter is to pass across all the functions to arrive the cluster create command, and we can do that with any code change as --wildcard-options "--tags=TicketId:XXXX

+1 to just moving to the wildcard option. I completely forgot about that in my previous comment. @morenod++

[krishvoor]

What I think is that this new parameter --ticket-id makes sense if we change it to required=True, and we validate somehow that it is a valid ticket. if it is going to be optional and directly passed to the create command, we should use the existing --wildcard-options one
What we decided about the parameters when we started to work on this wrapper was that we will only add parameters that we need to use, manage, transform or validate in the wrapper. Any parameter that is going to be directly passed to the rosa create command can be added in the --wildcard-options parameter.
For example, we dont have any parameter for the version of openshift to be installed, because we are not validating it on any way, so if you want to specify a version to install, you can use, for example --wildcard-options "--channel-group candidate --version 4.13.0-rc.4"
This is the same scenario, the only scope for this parameter is to pass across all the functions to arrive the cluster create command, and we can do that with any code change as --wildcard-options "--tags=TicketId:XXXX

+1 to just moving to the wildcard option. I completely forgot about that in my previous comment. @morenod++

@dry923 @morenod with --wildcard-options we will be limited to tag/target resources created during _build_cluster() and not operator_roles, oidc_objects, VPCs, machinepools.

@smalleni can we consider consolidating #157 and #159 into one?

[dry923]

What I think is that this new parameter --ticket-id makes sense if we change it to required=True, and we validate somehow that it is a valid ticket. if it is going to be optional and directly passed to the create command, we should use the existing --wildcard-options one
What we decided about the parameters when we started to work on this wrapper was that we will only add parameters that we need to use, manage, transform or validate in the wrapper. Any parameter that is going to be directly passed to the rosa create command can be added in the --wildcard-options parameter.
For example, we dont have any parameter for the version of openshift to be installed, because we are not validating it on any way, so if you want to specify a version to install, you can use, for example --wildcard-options "--channel-group candidate --version 4.13.0-rc.4"
This is the same scenario, the only scope for this parameter is to pass across all the functions to arrive the cluster create command, and we can do that with any code change as --wildcard-options "--tags=TicketId:XXXX

+1 to just moving to the wildcard option. I completely forgot about that in my previous comment. @morenod++

@dry923 @morenod with --wildcard-options we will be limited to tag/target resources created during _build_cluster() and not operator_roles, oidc_objects, VPCs, machinepools.

@smalleni can we consider consolidating #157 and #159 into one?

@krishvoor per my conversation with the cloud-gov folks they only care that the instance is tagged

[smalleni]

What I think is that this new parameter --ticket-id makes sense if we change it to required=True, and we validate somehow that it is a valid ticket. if it is going to be optional and directly passed to the create command, we should use the existing --wildcard-options one
What we decided about the parameters when we started to work on this wrapper was that we will only add parameters that we need to use, manage, transform or validate in the wrapper. Any parameter that is going to be directly passed to the rosa create command can be added in the --wildcard-options parameter.
For example, we dont have any parameter for the version of openshift to be installed, because we are not validating it on any way, so if you want to specify a version to install, you can use, for example --wildcard-options "--channel-group candidate --version 4.13.0-rc.4"
This is the same scenario, the only scope for this parameter is to pass across all the functions to arrive the cluster create command, and we can do that with any code change as --wildcard-options "--tags=TicketId:XXXX

+1 to just moving to the wildcard option. I completely forgot about that in my previous comment. @morenod++

@dry923 @morenod with --wildcard-options we will be limited to tag/target resources created during _build_cluster() and not operator_roles, oidc_objects, VPCs, machinepools.

@smalleni can we consider consolidating #157 and #159 into one?

@krishvoor I am not following this. Using --wild-card-options to pass --tags option with TicketId to rosa cli is no different from explicitly consuming and passing --tags TicketId. However as @morenod mentioned anything passed to -wildcard-options is not mandatory and does not undergo any additional checks, which is not different from how this patch uses --ticket-id argument.

[smalleni]

What I think is that this new parameter --ticket-id makes sense if we change it to required=True, and we validate somehow that it is a valid ticket. if it is going to be optional and directly passed to the create command, we should use the existing --wildcard-options one

What we decided about the parameters when we started to work on this wrapper was that we will only add parameters that we need to use, manage, transform or validate in the wrapper. Any parameter that is going to be directly passed to the rosa create command can be added in the --wildcard-options parameter.

For example, we dont have any parameter for the version of openshift to be installed, because we are not validating it on any way, so if you want to specify a version to install, you can use, for example --wildcard-options "--channel-group candidate --version 4.13.0-rc.4"

This is the same scenario, the only scope for this parameter is to pass across all the functions to arrive the cluster create command, and we can do that with any code change as --wildcard-options "--tags=TicketId:XXXX

I agree with David's assessment. The only case where we need --ticket-id as a separate argument is if we consider it mandatory aka required. I think given the direction we are moving with cloud budgeting and spend, it makes sense to enforce this argument. However, while I would like to make the TicketId mandatory for spinning up HCPs, I'm not exactly sure if have the necessary mechanisms to verify if the ticket is valid AND approved. Perhaps the clpud governance framework exposes an API to check this? @krishvoor can you check?

[krishvoor]

What I think is that this new parameter --ticket-id makes sense if we change it to required=True, and we validate somehow that it is a valid ticket. if it is going to be optional and directly passed to the create command, we should use the existing --wildcard-options one
What we decided about the parameters when we started to work on this wrapper was that we will only add parameters that we need to use, manage, transform or validate in the wrapper. Any parameter that is going to be directly passed to the rosa create command can be added in the --wildcard-options parameter.
For example, we dont have any parameter for the version of openshift to be installed, because we are not validating it on any way, so if you want to specify a version to install, you can use, for example --wildcard-options "--channel-group candidate --version 4.13.0-rc.4"
This is the same scenario, the only scope for this parameter is to pass across all the functions to arrive the cluster create command, and we can do that with any code change as --wildcard-options "--tags=TicketId:XXXX

I agree with David's assessment. The only case where we need --ticket-id as a separate argument is if we consider it mandatory aka required. I think given the direction we are moving with cloud budgeting and spend, it makes sense to enforce this argument. However, while I would like to make the TicketId mandatory for spinning up HCPs, I'm not exactly sure if have the necessary mechanisms to verify if the ticket is valid AND approved. Perhaps the clpud governance framework exposes an API to check this? @krishvoor can you check?

We can make use of JIRA API(s) to validate if a ticket is opened.
I have jotted something like this:
http://pastebin.test.redhat.com/1102940
which checks if an Issue ID is present.

[dry923]

What I think is that this new parameter --ticket-id makes sense if we change it to required=True, and we validate somehow that it is a valid ticket. if it is going to be optional and directly passed to the create command, we should use the existing --wildcard-options one
What we decided about the parameters when we started to work on this wrapper was that we will only add parameters that we need to use, manage, transform or validate in the wrapper. Any parameter that is going to be directly passed to the rosa create command can be added in the --wildcard-options parameter.
For example, we dont have any parameter for the version of openshift to be installed, because we are not validating it on any way, so if you want to specify a version to install, you can use, for example --wildcard-options "--channel-group candidate --version 4.13.0-rc.4"
This is the same scenario, the only scope for this parameter is to pass across all the functions to arrive the cluster create command, and we can do that with any code change as --wildcard-options "--tags=TicketId:XXXX

I agree with David's assessment. The only case where we need --ticket-id as a separate argument is if we consider it mandatory aka required. I think given the direction we are moving with cloud budgeting and spend, it makes sense to enforce this argument. However, while I would like to make the TicketId mandatory for spinning up HCPs, I'm not exactly sure if have the necessary mechanisms to verify if the ticket is valid AND approved. Perhaps the clpud governance framework exposes an API to check this? @krishvoor can you check?

We can make use of JIRA API(s) to validate if a ticket is opened. I have jotted something like this: http://pastebin.test.redhat.com/1102940 which checks if an Issue ID is present.

IMHO ^ should not be in this script. You can verify it outside and pass the "approved" value in in airflow or something but this would not align with the overall goal of the rosa-hypershift wrapper.

[krishvoor]

IMHO ^ should not be in this script. You can verify it outside and pass the "approved" value in in airflow or something but this would not align with the overall goal of the rosa-hypershift wrapper.

Ack, we will not taper the sanity of the wrapper, we can stage it in utils/* and pass the value accordingly.

[krishvoor]

Hi @dry923 @morenod @smalleni 👋
I have tackled it outside the scope of the wrapper script via #162
and checks for a "valid" JIRA Ticket and applies logic accordingly, please review.

[krishvoor]

Hi @smalleni, looks like you got some resolve conflicts, kindly resolve.

[krishvoor]

Why do we want to enforce this as required=True?

[krishvoor]

@smalleni: Having required=True, will compel users to provide ticket-id,
which may not be the case always, thoughts?