Add --env to comment trigger phrase #2583
Conversation
Frawless
force-pushed
the
add-envs-to-comment-trigger
branch
from
894950f
to
2a7b77b
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 13s |
| env_variables = { | ||
| k: v | ||
| for k, v in self.comment_arguments.envs.items() | ||
| if v is not None | ||
| } |
There was a problem hiding this comment.
what about a use-case of "unsetting" env var via comment arg?
There was a problem hiding this comment.
I didn't think about this one. You mean something like --env MY_ENV_VAR="" will unset it or do you have something else how should look like? It shouldn't be a problem to implement it.
There was a problem hiding this comment.
Or you mean if value is None then unset it?
There was a problem hiding this comment.
yes, practically both, like if someone passes --env ENV_VAR_TO_UNSET= --env OTHER_ENV_VAR=value, which would lead to the value being None
| # Set up argparse | ||
| parser = argparse.ArgumentParser() | ||
| parser.add_argument("packit_command") | ||
| parser.add_argument("--identifier", "--id", "-i") | ||
| parser.add_argument("--labels") | ||
| parser.add_argument("--env", action="append") # Allows multiple --env arguments |
There was a problem hiding this comment.
maybe we could create property out of this, or at least separate the creation of the parser
There was a problem hiding this comment.
I am not python expert so maybe my changes are not python compliant, but I think it is better now
| if args.env: | ||
| # Parse envs into dictionary | ||
| self.envs = dict(env.split("=") for env in args.env) | ||
| logger.debug(f"Parsed envs: {self.envs}") |
There was a problem hiding this comment.
I think here we should add some error handling for bad input (not key=value format)
There was a problem hiding this comment.
I did some changes, please take a look :)
| identifier: str = None | ||
| labels: List[str] = None | ||
| pr_argument: str = None | ||
|
|
||
| def __init__(self, command_prefix: str, comment: str): |
There was a problem hiding this comment.
the __init__ method grew quite a lot with the new code, do you think it could be split up a bit?
There was a problem hiding this comment.
Sure, I will try to split it
Frawless
force-pushed
the
add-envs-to-comment-trigger
branch
from
2a7b77b
to
27edb85
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 13s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 29s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 32s |
| env_variables = { | ||
| k: v | ||
| for k, v in self.comment_arguments.envs.items() | ||
| if v is not None | ||
| } |
There was a problem hiding this comment.
yes, practically both, like if someone passes --env ENV_VAR_TO_UNSET= --env OTHER_ENV_VAR=value, which would lead to the value being None
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 27s |
Frawless
force-pushed
the
add-envs-to-comment-trigger
branch
from
3a74c46
to
7a5657d
Compare
Signed-off-by: Jakub Stejskal <[email protected]>
Frawless
force-pushed
the
add-envs-to-comment-trigger
branch
from
7a5657d
to
7cc43b1
Compare
|
@lbarcziova I updated the code, please take a look if the unset functionality is fine. |
|
Build succeeded. ✔️ pre-commit SUCCESS in 12m 10s |
|
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 2m 29s |
softwarefactory-project-zuul
bot
merged commit 5622b78
into
packit:main
Checklist:
packit/packit.dev.This PR adds possibility to specify
--envparameters in manual trigger command.User can specify multiple env variables in format like
--env MY_ENV_VAR=value --env MY_ENV_VAR2=value2. Every env var has to have it's own--envkeyword.Env variables specified with this option has the highest priority when building TF payload.
Fixes #2200
RELEASE NOTES BEGIN
Packit now supports
--envparameters in manual trigger command.RELEASE NOTES END