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

Fix deprecation warnings #811

Merged
merged 3 commits into from
Nov 28, 2023
Merged

Conversation

mdellweg
Copy link
Member

This is yet another legacy of the pulp_file merge.

- name: Install requirements
run: pip3 install github
- name: Check commit message
{{ install_python_deps(["requests", "pygithub"]) | indent(6) }}
Copy link
Member

Choose a reason for hiding this comment

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

We install missing packages in the scripts themselves; the question is, do we need this^?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought i removed that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, i did.

@@ -74,7 +73,7 @@ jobs:

{{ setup_python() | indent(6) }}

{{ install_python_deps("requests") | indent(6) }}
{{ install_python_deps(["requests"]) | indent(6) }}
Copy link
Member

Choose a reason for hiding this comment

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

I would also install this requirement inside the caller ".github/workflows/scripts/publish_docs.sh". Do you think it is worth it to revisit all installing dependencies and move them to one place?

Copy link
Member Author

Choose a reason for hiding this comment

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

I intended to stop installing anything from scripts, because i consider that to be dangerous if you try to run any of these locally.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, then it makes sense to remove pip calls from the scripts, in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

And i dind't even check with "git grep" ... XD

{%- if test_lowerbounds %}
echo "{{ "${{ needs.test.outputs.deprecations-lowerbounds }}" }}" | base64 -d
{%- endif %}
cat deprecations-*.txt | sort -u
Copy link
Member

Choose a reason for hiding this comment

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

Do we miss "{{ plugin_name }}" in the path?

Copy link
Member Author

Choose a reason for hiding this comment

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

At this point no. As we do no checkout, we don't have the directory "{{ plugin_name }}" to do that. Do you think we should create it for consistency?

Copy link
Member

Choose a reason for hiding this comment

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

I would say yes.

Consolidate some macros. Add a lot of quoting because explicit is better
than implicit.

[noissue]
@mdellweg mdellweg force-pushed the fix_deprecation_warnings branch from 5b6d75a to 146b9c8 Compare November 28, 2023 13:19
@mdellweg mdellweg force-pushed the fix_deprecation_warnings branch from 720f438 to 5f9c44b Compare November 28, 2023 13:48
This will not rely on job outputs, but instead use a workflow artifact.

[noissue]
@mdellweg mdellweg force-pushed the fix_deprecation_warnings branch from 5f9c44b to b7dd772 Compare November 28, 2023 13:52
@mdellweg mdellweg merged commit 53132bb into pulp:main Nov 28, 2023
16 checks passed
@mdellweg mdellweg deleted the fix_deprecation_warnings branch November 28, 2023 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants