-
Notifications
You must be signed in to change notification settings - Fork 23
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 feature to submit to Factory or staging based on flag #380
base: master
Are you sure you want to change the base?
Conversation
I submit some changes which make sense to me but i see space for improvement. I test this with but I would need some help to test it properly. |
0f0902b
to
be18b68
Compare
os-autoinst-obs-auto-submit
Outdated
@@ -5,6 +5,7 @@ set -euo pipefail | |||
export LC_ALL=C | |||
src_project=${src_project:-devel:openQA} | |||
dst_project=${dst_project:-${src_project}:tested} | |||
submit_to_factory="${submit_to_factory:-1}" |
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 suggest you avoid useless quotes, especially since most of the other assignments avoid them, too:
submit_to_factory="${submit_to_factory:-1}" | |
submit_to_factory=${submit_to_factory:-1} |
os-autoinst-obs-auto-submit
Outdated
@@ -5,6 +5,7 @@ set -euo pipefail | |||
export LC_ALL=C | |||
src_project=${src_project:-devel:openQA} | |||
dst_project=${dst_project:-${src_project}:tested} | |||
submit_to_factory="${submit_to_factory:-1}" | |||
staging_project=${staging_project:-${src_project}:testing} | |||
submit_target="${submit_target:-"openSUSE:Factory"}" |
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.
This is probably what you want to change here (from the outside, when invoking this script). So maybe there's no need to introduce submit_to_factory
and just decide what to do based on submit_target
? This would eliminate the case when submit_to_factory
and submit_target
are in disagreement.
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.
good point. 🙏
os-autoinst-obs-auto-submit
Outdated
exit 0 | ||
fi | ||
is_staging_specified() { | ||
[[ -n "$staging_project" && "$staging_project" != "none" && "$staging_project" != "$src_project" ]] && ( |
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.
Please avoid the unnecessary quoting. Only the right hand side needs to be quoted when using [[ … ]]
so keep the quoting as it was before.
I guess you're on the right track with extracting some of the code into functions that via unit tests then. Otherwise I'm normally also just invoking the script using dry runs. Sometimes I'm also just testing sections or single lines by pasting then into a local terminal or a smaller temporary script. This is of course not ideal. |
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 am not sure if we are going the right direction here. How about looking into AMQP event based triggering and acting upon SRs acceptance to openSUSE:Factory and then simply forward to the most recent backports project?
- Separate first submission to factory from anything else. By default use the submit_target variable which targets openSUSE:Factory - Refactor factory_request to take extra variables and make the query more abstract and modular - Encapsulate logic in a main and break it into smaller logical functions The automatic submission is designed to sumbmit to the Factory using staging repo. However after that there is no need of this. The packages can be submitted to any other project from factory once they are accepted in there. https://progress.opensuse.org/issues/127037
submit_from_staging
. Maybe those two can merged later.https://progress.opensuse.org/issues/127037