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

Require target-name parameter #430

Closed
renatav opened this issue May 23, 2024 · 5 comments · Fixed by #442
Closed

Require target-name parameter #430

renatav opened this issue May 23, 2024 · 5 comments · Fixed by #442
Assignees
Labels
good first issue Good for newcomers

Comments

@renatav
Copy link
Collaborator

renatav commented May 23, 2024

taf targets add-repo --target-name parameter should be a required path argument

@renatav renatav added the good first issue Good for newcomers label May 23, 2024
@renatav renatav changed the title taf targets add-repo --target-name parameter should be a required path argument Require target-name parameter May 23, 2024
@Rana-KV
Copy link
Contributor

Rana-KV commented Jun 8, 2024

Hi @renatav, I would like take up this issue and work on it.

@renatav
Copy link
Collaborator Author

renatav commented Jun 8, 2024

@Rana-KV Awesome, I assigned it to you,

@Rana-KV
Copy link
Contributor

Rana-KV commented Jun 8, 2024

Thanks for assigning the issues @renatav
I have question regarding the changes. If target-name is required, then can I remove the fallback option to use target-path? or both are required?

TAF-Capture

@renatav
Copy link
Collaborator Author

renatav commented Jun 8, 2024

@Rana-KV We tend to follow a certain convention regarding where the repositories are located. Their filesystem paths tend to include their names. This will be the case if you use the updater to clone the repositories, like it is intended. So, if you have a name, the chances are that we can also correctly calculate the path. If for whatever reason that is not the case, we probably should still make it possible to specify a different path. So, let's actually reverse the order of this if and elif instead of removing the elif

@Rana-KV
Copy link
Contributor

Rana-KV commented Jun 8, 2024

Thanks for the information @renatav, I have reversed the order and still kept the description in targest.py about target-name parameter as optional because it be still done with just taget-path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants