Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
add v2 recipe editing capabilities for bumping version & build number #2920
base: main
Are you sure you want to change the base?
add v2 recipe editing capabilities for bumping version & build number #2920
Changes from 8 commits
dc9ef01
e5751ef
4c83a3b
ee9497f
eed8c2e
b818676
d109abd
b7f6136
58596d2
4b3d827
d0681fb
1f0fba7
0882421
7fee6de
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 574 in conda_forge_tick/migrators/core.py
Codecov / codecov/patch
conda_forge_tick/migrators/core.py#L574
Check warning on line 220 in conda_forge_tick/migrators/version.py
Codecov / codecov/patch
conda_forge_tick/migrators/version.py#L216-L220
Check warning on line 9 in conda_forge_tick/update_recipe/v2/build_number.py
Codecov / codecov/patch
conda_forge_tick/update_recipe/v2/build_number.py#L9
Check warning on line 38 in conda_forge_tick/update_recipe/v2/build_number.py
Codecov / codecov/patch
conda_forge_tick/update_recipe/v2/build_number.py#L35-L38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People can and have mixed setting build numbers in both the jinja2 vars / context and explicitly in the recipe. I know it is awful. We should update the code to do both and check if the build number in the non-context part is templated or not before updating.
Check warning on line 6 in conda_forge_tick/update_recipe/v2/conditional_list.py
Codecov / codecov/patch
conda_forge_tick/update_recipe/v2/conditional_list.py#L6
Check warning on line 40 in conda_forge_tick/update_recipe/v2/conditional_list.py
Codecov / codecov/patch
conda_forge_tick/update_recipe/v2/conditional_list.py#L40
Check warning on line 57 in conda_forge_tick/update_recipe/v2/conditional_list.py
Codecov / codecov/patch
conda_forge_tick/update_recipe/v2/conditional_list.py#L54-L57
Check warning on line 62 in conda_forge_tick/update_recipe/v2/conditional_list.py
Codecov / codecov/patch
conda_forge_tick/update_recipe/v2/conditional_list.py#L62
Check warning on line 69 in conda_forge_tick/update_recipe/v2/conditional_list.py
Codecov / codecov/patch
conda_forge_tick/update_recipe/v2/conditional_list.py#L69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned that assuming linux as the default will result in invalid updates for some recipes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand where you are coming from but we do ignore the selectors in the version updating code (e.g. we go down both branches,
then
andelse
.However, we do use the value for templating, so people could theoretically do something like
But that doesn't seem very likely and also buggy so I don't think we'll see that a lot in the wild.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are folks allowed to use if..then in the context section to change the values of variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that doesn't work in the context section. However you can use templating and inline Jinja expressions, such as:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right so we need to make this correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That won't change other items that might be platform dependent in the context section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the SHA hash will make it impossible, thankfully :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. Folks could define parts of urls in the context section that are platform dependent that are not hashes. So when the bot comes through to render those urls and parts, it will compute the wrong hash since it might have a valid, but incorrect url for that platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beckermr - i sorta get where you come from but is it really necessary to care about all of these edge cases on day one?
It is also not a very clever way to write the recipe. You would have to "double-if" the source.
E.g.
So I don't think many people would wanna write their sources like this (doesn't make much logical sense).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we should care. My suggested procedure for this work was to abstract the current version update algorithm to be independent of the recipe format backend and then use that. It should handle most, if not all, of the cases I am bringing up.
Check warning on line 12 in conda_forge_tick/update_recipe/v2/jinja/filters.py
Codecov / codecov/patch
conda_forge_tick/update_recipe/v2/jinja/filters.py#L11-L12
Check warning on line 14 in conda_forge_tick/update_recipe/v2/jinja/filters.py
Codecov / codecov/patch
conda_forge_tick/update_recipe/v2/jinja/filters.py#L14
Check warning on line 19 in conda_forge_tick/update_recipe/v2/jinja/filters.py
Codecov / codecov/patch
conda_forge_tick/update_recipe/v2/jinja/filters.py#L16-L19
Check warning on line 23 in conda_forge_tick/update_recipe/v2/jinja/filters.py
Codecov / codecov/patch
conda_forge_tick/update_recipe/v2/jinja/filters.py#L23
Check warning on line 28 in conda_forge_tick/update_recipe/v2/jinja/filters.py
Codecov / codecov/patch
conda_forge_tick/update_recipe/v2/jinja/filters.py#L28