This could be confusing (not possible to validate -> the validation service is not responding?). How about Found a gating.yaml file in your repo, but it is not valid. Please fix the file or skip this check using the parameter --skip-remoterules-validation?

No other option is using underscores. Maybe make it consistent?

config.has_option could cover two potential problem, no section or no url option in section %s.greenwave.

line 33 would fail if there is misconfiguration of url.

Please use with statement to ensure file is closed.

Another issue is data would be bytestring, is this expected to pass to validate-gating-yaml?

Data is a string. Is this correct?

Suggestion: to reduce one level indentation, move this if out of check_remoterules_gating. Hence, if greenwave_url is None, do not call check_remoterules_gating.

I think check_remote_rules_gating would be easier to read.

What is it used for to add skip_remoterules_validation to opts?

I think check_remote_rules_gating would be easier to read.

I see remoterules is used in other places. If it is expected, it's ok to keep this unchanged.

Would it be good to mention greenwave in this message? It would be helpful to make it clear to understand what are the remote rules for. I think Not all packagers are familiar with greenwave, once they see this message, they could confuse and no information to help them to know what would happen by using this option.

Alternatively, option --skip-remoterules-validation could be more informative as well.

Both build and scratch-build commands go into build method. Does gating.yaml need to be validated for scratch build?

Any reason to check gating.yaml inside Commands? Generally, cliClient is a good place to apply kinds of checks like this as early as possible to determine if the command should be proceeded, and then corresponding method in Commands is responsible for doing the real job.

In this case, the workflow would be

class cliClient(object):
    def check_remoteurls_gating(self):
        if (self.args.skip_remoterules_validation or 
                not os.path.exists(os.path.join(self.path, 'gating.yaml')) or
                not self.config.has_option('fedpkg.greenwave', 'url')):
            return
        # read content from gating.yaml and validate it from greenwave endpoint
def build(self):
    ...
    check_remoteurls_gating()
    ...

self.args.scratch and self.args.chain could be used to filter out unnecessary type of build in build method.

As you can see, the logic is clear and easier to understand the validation policy without reading throught the code until to Commands.build, meanwhile we also don't need additional parameter greenwave_url passed.

This is a suggestion to refactor your code. FYI.

Yes, it is.

Bytestring would not be a problem for validate-gating-yaml.

It will eventually in the future, but not right now. So maybe I can check if it is a scratch build.

Your thought is correct I think. But, if you mock requests.post, you can cover more lines of code including asserting if the request to greenwave endpoint is correct with proper request parameters.

But, I'm confused what this test tests. gating.py is written, should this test test not validate gating.yaml? And it looks test name test_greenwave_remoterule is not informative enough.

Hi @gnaponie

Can you point an example of gating.yaml to me? Either an example file or a package repo containing such a file is fine. Thank you.

@cqi @lsedlar thank you for the suggestions. I'm doing all the corrections.

@cqi I used this repo in the past to make some tests about the Greenwave RemoteRule feature:
https://src.stg.fedoraproject.org/rpms/python-ansi2html

Here you can see a correct gating.yaml file: https://src.stg.fedoraproject.org/rpms/python-ansi2html/blob/b7df3e36efe46620155994bd8c71059c210eccd4/f/gating.yaml
and here you can see a misconfigured (just for testing purposes) gating.yaml file (RemoteRule is not accepted in such file): https://src.stg.fedoraproject.org/rpms/python-ansi2html/c/0a2380c85b22a1702039d02bf03867ffacf8931d

Any reason to check gating.yaml inside Commands? Generally, cliClient is a good place to apply kinds of checks like this as early as possible to determine if the command should be proceeded, and then corresponding method in Commands is responsible for doing the real job.
In this case, the workflow would be
class cliClient(object):

def check_remoteurls_gating(self):
    if (self.args.skip_remoterules_validation or 
            not os.path.exists(os.path.join(self.path, 'gating.yaml')) or
            not self.config.has_option('fedpkg.greenwave', 'url')):
        return
    # read content from gating.yaml and validate it from greenwave endpoint

def build(self):
...
check_remoteurls_gating()
...

self.args.scratch and self.args.chain could be used to filter out unnecessary type of build in build method.
As you can see, the logic is clear and easier to understand the validation policy without reading throught the code until to Commands.build, meanwhile we also don't need additional parameter greenwave_url passed.
This is a suggestion to refactor your code. FYI.

I just put it there because I had this available there: "self.repo.working_tree_dir". But I guess I can also use self.path.

Thanks

Any reason to check gating.yaml inside Commands? Generally, cliClient is a good place to apply kinds of checks like this as early as possible to determine if the command should be proceeded, and then corresponding method in Commands is responsible for doing the real job.
In this case, the workflow would be
class cliClient(object):
def check_remoteurls_gating(self):
if (self.args.skip_remoterules_validation or
not os.path.exists(os.path.join(self.path, 'gating.yaml')) or
not self.config.has_option('fedpkg.greenwave', 'url')):
return
# read content from gating.yaml and validate it from greenwave endpoint

def build(self):
...
check_remoteurls_gating()
...
self.args.scratch and self.args.chain could be used to filter out unnecessary type of build in build method.
As you can see, the logic is clear and easier to understand the validation policy without reading throught the code until to Commands.build, meanwhile we also don't need additional parameter greenwave_url passed.
This is a suggestion to refactor your code. FYI.

I just put it there because I had this available there: "self.repo.working_tree_dir". But I guess I can also use self.path.
Thanks

Actually in that point self.path does not exist. I guess I cannot assume that the path repo will always be the current directory. I tried to look into "self", looking for an alternative, I didn't find it. I'll continue to look.

I just put it there because I had this available there:
"self.repo.working_tree_dir". But I guess I can also use self.path.

Yeah, you are right. :)

Actually in that point self.path does not exist. I guess I cannot assume
that the path repo will always be the current directory. I tried to look
into "self", looking for an alternative, I didn't find it. I'll continue to
look. ``

In cliClient, it's self.args.path.

awesome! Thank you @cqi
Very helpful

I wanted the test to:
- test if the build is stopped if there is a misconfigured gating.yaml file
- test if the build isn't stopped if there is a correct gating.yaml file
- test that with a misconfigured gating.yaml file the build is not blocked if the "--skip-remote-rules-validation" is provided

Rebased with the suggestions.

Assertion on session.build would be better. It is the method to request a build, so it should be easier to understand.

Is this line required? If no, let's remove it instead of a comment.

This test name is not informative enough to tell what this test tests. How about rename it to test_not_allow_to_build_if_gating_yaml_is_invalid?

Should we handle specific status code differently? I'm not sure my understand of validate_gating_yaml_post is correct. If gating.yaml is invalid, response is BadRequest, otherwise it's ok (200).

If yes, should only the BadRequest response mean an invalid gating.yaml? Other type of response, like 500 or 401, should be handled differently? For example, just proceed to build and log a message, or something else.

On the other hand, there is --skip-remote-rules-validation to bypass the check. It would be good to tell this option when greenwave does not response BadRequest or OK.

Thanks @gnaponie. I also suggest you write additional tests to cover more cases. What I can think of are:

• test to ensure build is proceeded if gating.yaml is valid.
• test to ensure the check could be bypassed by specifying --skip-remote-rules-validation.
• test to ensure non BadRequest or OK response is handled as expected.

Feel free to add other tests if you think they are necessary.

@cqi thank you again for your feedback.
Rebased again. I believe now it should be ok.

Isn't this already handled in extract_greenwave_url method? Also the argument is checked there.

Looks good to me. :thumbsup:

@lsedlar thank you for your comment. I've fixed it.

Looks good to me. :thumbsup:

BTW, WIP could be removed from title, and it would be nice to rebase on master branch.

Thanks. Merging :tada: