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

Rewrite openqa-advanced-retrigger-jobs in python #332

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

okurz
Copy link
Member

@okurz okurz commented Jul 23, 2024

No description provided.

perlpunk

This comment was marked as resolved.

@okurz

This comment was marked as resolved.

@perlpunk

This comment was marked as resolved.

@perlpunk
Copy link
Contributor

Also how about moving scripts to bin/ and adding an extension, .py. I don't have a problem with extensions, and it makes tooling like linting easier

@okurz
Copy link
Member Author

okurz commented Jul 23, 2024

Also how about moving scripts to bin/ and adding an extension, .py. I don't have a problem with extensions, and it makes tooling like linting easier

An extension would mean that we would need a separate installation tooling and step to install as executable without extension and I would like to avoid that.

@okurz okurz force-pushed the feature/openqa_advanced_retrigger_python2 branch from f32c1d5 to f8d55f9 Compare July 24, 2024 07:00
@okurz
Copy link
Member Author

okurz commented Jul 24, 2024

It would still be good to have an example. I remember that it was complicated to get the quoting right

Done

openqa-investigate Outdated Show resolved Hide resolved
openqa-investigate Outdated Show resolved Hide resolved
openqa-investigate Outdated Show resolved Hide resolved
@okurz okurz force-pushed the feature/openqa_advanced_retrigger_python2 branch from f8d55f9 to d00d067 Compare July 24, 2024 10:39
@okurz okurz force-pushed the feature/openqa_advanced_retrigger_python2 branch 2 times, most recently from bd97f6e to bdd1863 Compare July 25, 2024 18:12
@okurz
Copy link
Member Author

okurz commented Jul 25, 2024

Updated:

  • Simple and advanced example
  • Converted single string in subprocess.run with shell=True into list (or actually a set). For that I extracted the post function so that I can just print in case of "dry-run" instead of the "client prefix" which was previously used to "echo" instead in bash.

Tested manually with

while_inotifywait sh -c "python3.11 ./openqa-advanced-retrigger-jobs -vvvv --host openqa.suse.de --failed-since \"2024-07-23T13:00\" --result failed --additional-filters \"test not like '%:investigate:%' and id=14995768\" --comment \"my foo comment\" --dry-run && python3.11 ./openqa-advanced-retrigger-jobs -vvvv --host openqa.suse.de --failed-since \"2024-07-23T13:00\" --result failed --additional-filters \"test not like '%:investigate:%' and id=14995768\" --comment \"my foo comment\""

@okurz okurz removed the not-ready label Jul 25, 2024
Comment on lines +89 to +85
query = (
f"select id from jobs where ({worker_string}result='{args.result}' "
f"and clone_id is null and t_finished >= '{args.failed_since}'{additional_filters});"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally I'd say this needs escaping but of course the previous script also didn't have 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.

I don't know what additional escaping you mean. Why do you mean we need escaping?

Copy link
Member

Choose a reason for hiding this comment

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

I think he means something in the lines of https://www.psycopg.org/psycopg3/docs/basic/params.html#execute-arguments

Currently e.g. args.result could be used to insert arbitrary SQL statements

Copy link
Member

Choose a reason for hiding this comment

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

While malice is very unlikely (given you'd need access anyway) it could help if people accidentally pass arguments which cause the query to break

Copy link
Contributor

Choose a reason for hiding this comment

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

And one can also (accidentally) break out of the whole psql invocation.

openqa-advanced-retrigger-jobs Outdated Show resolved Hide resolved
openqa-advanced-retrigger-jobs Outdated Show resolved Hide resolved
@okurz okurz force-pushed the feature/openqa_advanced_retrigger_python2 branch from 9725eaf to 00ab178 Compare July 29, 2024 11:58
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.

5 participants