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

Replace repo_root_path with dci_cluster_configs_dir pinned in DOA #140

Closed
wants to merge 1 commit into from

Conversation

tkrishtop
Copy link
Contributor

@tkrishtop tkrishtop commented Jan 19, 2024

Test-Hints: assisted-abi

Depends-On: https://github.com/dci-labs/inventories/pull/195

This is an attempt to fix the bug described here

@tkrishtop tkrishtop requested a review from a team as a code owner January 19, 2024 14:02
@tkrishtop tkrishtop marked this pull request as draft January 19, 2024 14:03
@dcibot
Copy link
Collaborator

dcibot commented Jan 19, 2024

Starting dci-check-change job.

Copy link

@dcibot
Copy link
Collaborator

dcibot commented Jan 19, 2024

@tkrishtop
Copy link
Contributor Author

tkrishtop commented Jan 19, 2024

recheck

@tkrishtop tkrishtop changed the title [DNM] Replace repo_root_path with dci_cluster_configs_dir pinned in DOA Replace repo_root_path with dci_cluster_configs_dir pinned in DOA Jan 22, 2024
@tkrishtop
Copy link
Contributor Author

recheck

Copy link

@nocturnalastro
Copy link
Contributor

We have to think of backward compatibility with pre-existing crucible inventories. What about defaulting repo_root_path to dci_cluster_configs_dir. then in the inventory validation we can check if either are defined.
That way dci can use dci_cluster_configs_dir and we don't break anyone's inventories.

@tkrishtop
Copy link
Contributor Author

We have to think of backward compatibility with pre-existing crucible inventories. What about defaulting repo_root_path to dci_cluster_configs_dir. then in the inventory validation we can check if either are defined.

Thank you @nocturnalastro, I see the point. Let's do as you propose, let me push the changes.

@dcibot
Copy link
Collaborator

dcibot commented Jan 22, 2024

Starting dci-check-change job.

Copy link

@dcibot
Copy link
Collaborator

dcibot commented Jan 22, 2024

@tkrishtop tkrishtop marked this pull request as ready for review January 22, 2024 20:49
@tkrishtop
Copy link
Contributor Author

We have to think of backward compatibility with pre-existing crucible inventories. What about defaulting repo_root_path to dci_cluster_configs_dir. then in the inventory validation we can check if either are defined.

Thank you @nocturnalastro, I see the point. Let's do as you propose, let me push the changes.

Hi @nocturnalastro, I made the changes you requested and tested on assisted-abi installation: https://www.distributed-ci.io/jobs/ff803bb8-a088-464f-b40e-872981a2b26d/jobStates?sort=date

Could you please have a look one more time?

@tonyskapunk
Copy link
Contributor

I'm not sure the roles should use a dci variable coming from the dci openshift agent. Would it be possible to make repo_root_path: dci_cluster_configs_dir as a var when called from the agent instead?

@ramperher
Copy link
Contributor

I'm not sure the roles should use a dci variable coming from the dci openshift agent. Would it be possible to make repo_root_path: dci_cluster_configs_dir as a var when called from the agent instead?

Yes, I agree with @tonyskapunk , maybe it's better to set up that variable when calling these roles from the agent.

@tkrishtop
Copy link
Contributor Author

I'm not sure the roles should use a dci variable coming from the dci openshift agent. Would it be possible to make repo_root_path: dci_cluster_configs_dir as a var when called from the agent instead?

Hi @tonyskapunk thanks for the idea, makes sense. Let me try it out.

@dcibot
Copy link
Collaborator

dcibot commented Jan 24, 2024

Starting dci-check-change job.

@dcibot
Copy link
Collaborator

dcibot commented Jan 24, 2024

@thekad
Copy link
Contributor

thekad commented Jan 24, 2024

agreed with @tonyskapunk these roles are not DCI specific so they shouldn't rely on DCI variables. There's two ways you can do this:

  • Do what Tony said and alter it on the calling playbook (easier, cleaner)
  • Add an extra variable default_repo_root_path and alter that one (somewhat dirtier, but more flexible maybe)

I'd go with Tony's way, and only adjust these roles in a non-DCI way.

@tkrishtop
Copy link
Contributor Author

agreed with @tonyskapunk these roles are not DCI specific so they shouldn't rely on DCI variables. There's two ways you can do this:

  • Do what Tony said and alter it on the calling playbook (easier, cleaner)
  • Add an extra variable default_repo_root_path and alter that one (somewhat dirtier, but more flexible maybe)

I'd go with Tony's way, and only adjust these roles in a non-DCI way.

agree, I'm testing with the current way just to ensure that all the dependencies are taken into account. Once it's green, I'll move the repo_root_path definition in the agent during the roles' invocation.

@tkrishtop
Copy link
Contributor Author

check dallas ocp-4.14-vanilla-abi

@tkrishtop tkrishtop marked this pull request as draft January 25, 2024 15:13
@dcibot
Copy link
Collaborator

dcibot commented Jan 25, 2024

@tkrishtop
Copy link
Contributor Author

@tkrishtop tkrishtop closed this Feb 7, 2024
@tonyskapunk tonyskapunk deleted the kubeconfig branch May 1, 2024 20:42
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.

6 participants