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

feat(core): extended template variable functionality #2120

Merged
merged 23 commits into from
Jan 11, 2022

Conversation

Panaetius
Copy link
Member

@Panaetius Panaetius commented May 31, 2021

Adds support for default values and typing (string, enum, boolean, number) to template variables.

https://dev.renku.ch/gitlab/renku-qa/core-it-template-variable-test-project contains an example of this.

closes #2034

/deploy

@Panaetius Panaetius requested a review from a team as a code owner May 31, 2021 14:21
@rokroskar
Copy link
Member

cc @lorenzo-cavazzi

@Panaetius Panaetius force-pushed the 2034-extended-template-variables branch 2 times, most recently from c9af8c3 to a64209c Compare June 1, 2021 09:49
@Panaetius Panaetius force-pushed the 2034-extended-template-variables branch from a64209c to e31efe3 Compare June 1, 2021 11:02
@lorenzo-cavazzi
Copy link
Member

lorenzo-cavazzi commented Jun 1, 2021

Great to see this taking form!

I was messing around a bit, and I've noticed that --description doesn't work well when I provide a specific source. In other words, renku init -d worked as expected showing me the description, while renku init -s <source> -d initialize the project ignoring the -d switch.

Should --description also show the expected type (if any)?

Screenshot_20210601_143223

@Panaetius
Copy link
Member Author

@lorenzo-cavazzi Oh actually that was a bug that has been around for a long time it seems, -d didn't work with a template repo that only has 1 template (In the case of 1 template, that one is used and the user is not prompted to pick a template, so the -d code was bypassed).

Not really a part of this ticket but I fixed it so you can test 🙂

@lorenzo-cavazzi
Copy link
Member

This works well in the cli 👍
Could you add the /deploy string in the first post? I'd like to quickly try it on the service

@Panaetius Panaetius temporarily deployed to renku-ci-rp-2120 June 2, 2021 14:21 Inactive
@RenkuBot
Copy link
Contributor

RenkuBot commented Jun 2, 2021

You can access the deployment of this PR at https://renku-ci-rp-2120.dev.renku.ch

@Panaetius Panaetius force-pushed the 2034-extended-template-variables branch from bbdd5d9 to 2898c36 Compare June 3, 2021 11:11
@Panaetius Panaetius temporarily deployed to renku-ci-rp-2120 June 3, 2021 11:11 Inactive
@lorenzo-cavazzi
Copy link
Member

Sorry for taking a bit longer for the review. I expected the core service to be compatible with the UI since no breaking changes have been introduced, but it crashes when creating a new project so I'm trying to go deeper into the problem.

In the meanwhile, I've noticed the CLI doesn't ask for a value when the parameter has a default. The only way to change it is by providing it through the console.
Would It be a better UX asking anyway for a value and using the default only if the user doesn't enter anything?

Screenshot_20210604_115315

Funny fact: the cli shows the parameters in different orders every time. It's not a big deal, but I found it a bit unusual (I typed fake description a few times before realizing it was asking me for the numeric variable first, since the first 3-4 times I got asked for the description first)

@lorenzo-cavazzi
Copy link
Member

This looks good to me! I tested a bit with https://dev.renku.ch/gitlab/lorenzo.cavazzi.tech/template-variables

It contains a breaking change for the UI since the templates.read_manifest API returns an object for each variable instead of a string, causing the UI to crash with all the templates.
Since that seems a good solution, I would try to have a compatible version of the APIs but I would wait to merge this until the UI catches up.

@Panaetius Panaetius temporarily deployed to renku-ci-rp-2120 June 7, 2021 08:24 Inactive
@rokroskar
Copy link
Member

Is it a breaking change or can it be merged independent of the UI PR?

@Panaetius
Copy link
Member Author

It should be backwards compatible (otherwise tests would fail as current templates don't contain this yet).

Copy link
Contributor

@m-alisafaee m-alisafaee left a comment

Choose a reason for hiding this comment

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

Looks very good! I've made some comments. I also notice that --describe gets ignored when passed without -l. This is a bit confusing. Perhaps we should rename it to --verbose or make also list and describe templates. This of course should be done in a new story.

renku/core/commands/init.py Outdated Show resolved Hide resolved
tests/cli/test_init.py Show resolved Hide resolved
@Panaetius Panaetius changed the base branch from master to develop January 3, 2022 14:42
@Panaetius Panaetius temporarily deployed to renku-ci-rp-2120 January 3, 2022 14:42 Inactive
@Panaetius Panaetius temporarily deployed to renku-ci-rp-2120 January 3, 2022 15:06 Inactive
@Panaetius Panaetius temporarily deployed to renku-ci-rp-2120 January 10, 2022 08:27 Inactive
Copy link
Contributor

@m-alisafaee m-alisafaee left a comment

Choose a reason for hiding this comment

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

Thank you!

@m-alisafaee m-alisafaee deployed to renku-ci-rp-2120 January 11, 2022 13:02 Active
@Panaetius Panaetius enabled auto-merge (squash) January 11, 2022 13:55
@Panaetius Panaetius merged commit 0e13fc1 into develop Jan 11, 2022
@Panaetius Panaetius deleted the 2034-extended-template-variables branch January 11, 2022 14:03
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.

Extend variables support in templates used by the init command
5 participants