I believe we want to use different URL for the side-tag artifact types ("brew-build-group" internally) but still keep the current dist-git URL for other artifact types.

MAX_RETRY and timeout should be in configuration.

Join with single space - it will be easier to copy the command.

Is there other way to find out if the file does not exist? Checking error string is not a good idea.

You did good changing the name, but we really need to remember to change the conf both internally and in fedora.

Yeah, that's true. Can we maybe keep DIST_GIT_BASE_URL and DIST_GIT_URL_TEMPLATE and rename the REMOTE_RULE_GIT_BASE_URL and REMOTE_RULE_GIT_URL_TEMPLATE to be more specific about the group artifacts? Maybe just GROUP_REMOTE_RULE_GIT_BASE_URL and GROUP_REMOTE_RULE_GIT_URL_TEMPLATE.

And then when we'll work on FACTORY-5058 we'll have to use DIST_GIT_* when people need to retrieve "standard" remote rules and GROUP_REMOTE_RULE_GIT_* when people need to retrieve group remote rules...

Maybe this shouldn't mention dist-git, but just say "remote repo".

Optional: this comment is correct. But the user might not know that for rules hosted on dist-git we use http and for rules for groups we use git archive. Maybe you could keep it like that and add in parenthesis this info (if they are hosted on dist-git or if they are about group rules).

I think it would be ideally something generic like:

# Maps artifact type to remote rule base URL.
REMOTE_RULE_GIT_BASE_URLS = {
    "brew-build-group": "https://gitlab.cee.redhat.com/devops/greenwave-policies/side-tags.git",
}
# Fallback URL for artifact types not in REMOTE_RULE_GIT_BASE_URLS.
REMOTE_RULE_GIT_BASE_URL_FALLBACK = "https://src.fedoraproject.org/"

I think I'd prefer to have in that repo only:
'{pkg_namespace}/{pkg_name}.yaml'
...having "_gating" seems redundant to me since this new repo is made only for these policies and most likely it won't contain other kind of files in there.

Remove this function, just use the retrieve_yaml_remote_rule_*() directly -- having a boolean flag as argument, as in this case, makes the code hard to read (especially in caller).

It's not clear from the docstring nor the function name that this function is specifically for side-tags.

Also, it will be difficult to add support for any new artifact type to use another git repo (in the greenwave-policies GitLab group).

Why not use a mapping for the configuration as I mentioned before (REMOTE_RULE_GIT_BASE_URLS)?

Could you squash the commits in only one?

Could you squash the commits in only one?

Of course, I'll squash it, once I got '+1'

Updated

Minor: this _retrieve_gating_yaml_error is used also here, but with the git-archive approach you shouldn't use dist-git.
So maybe we can change _retrieve_gating_yaml_error to be just something like: "Error occurred looking for the remote rules file in the repo."

It looks good, just a really minor comment. +1
@lholecek do you wanna check it too one last time?

Changing the str.format() parameter names also breaks the previous configuration.

Can we just assume git is installed and drop this? The function is too complex even without it.

This should now depend on the artifact/subject type.

Changing the str.format() parameter names also breaks the previous configuration.

no it doesn't (see replace in policies.py)

This should now depend on the artifact/subject type.

It's just a check if everything is configured properly, which doesn't care about the subject type.

This should now depend on the artifact/subject type.

It's just a check if everything is configured properly, which doesn't care about the subject type.

If user wants the URL for particular subject type, they don't necessarily require URL * -- i.e. with current code, it won't work if the URL is defined for the given subject type but undefined for *.

I don't see patterns being useful in the config dict -- it makes things much more complicated.

Is this useful by itself? Maybe just expand it in HTTP_URL_TEMPLATE below?

If user wants the URL for particular subject type, they don't necessarily require URL * -- i.e. with current code, it won't work if the URL is defined for the given subject type but undefined for *.

Why they shouldn't use something by default? For example if there is only one subject type it could be used as '*'...

Is this useful by itself? Maybe just expand it in HTTP_URL_TEMPLATE below?

Merged URL template with a base URL

If user wants the URL for particular subject type, they don't necessarily require URL * -- i.e. with current code, it won't work if the URL is defined for the given subject type but undefined for *.

Why they shouldn't use something by default? For example if there is only one subject type it could be used as '*'...

Why are they required to use "*" pattern? Why not simply drop the wildcard support and fallback to DIST_GIT_* options:

DIST_GIT_BASE_URL = 'https://src.fedoraproject.org/'
DIST_GIT_URL_TEMPLATE = '{DIST_GIT_BASE_URL}{pkg_namespace}/{pkg_name}/raw/{rev}/f/gating.yaml'
KOJI_BASE_URL = 'https://koji.fedoraproject.org/kojihub'
REMOTE_RULE_POLICIES = {
    'brew-build-group': {
        'GIT_URL': 'git@gitlab.cee.redhat.com:devops/greenwave-policies/side-tags.git',
        'GIT_PATH_TEMPLATE': '{pkg_namespace}/{pkg_name}.yaml'
    },
}

In subject type configuration, supports_remote_rule is False by default.

The new subject types for side-tags either need to be defined in conf/subject_types/ or there needs to be additional logic when getting supports_remote_rule property?

"*" is no longer mandatory

Oh, why was invalid-gating-yaml renamed? This potentially breaks the functionality for someone.

Looks like this branch is redundant now.

There could be a default pattern "*".

You can remove this sentence.

I'm not a fan of deprecating the old configuration - I wouldn't bother removing it in near future.

Also using patterns in the new config dict can lead to some unexpected behavior because someone may define brew-build-group before brew* but in Python 3.6 the order of values in the dict is undefined. Also, the example I gave is pretty useless, I really cannot think about what I would use the patterns for.

Let's wait for Giulia to review ... and we can talk about it later.

In the new repo the file with the policies will not be called "gating.yaml" anymore, so I think that's the reason behind it. But yeah, it would break the functionality... I would just leave "invalid-gating-yaml" as Lukas pointed out just for this one.

I think we should it keep it backward compatible. We could maintain also the old configuration without deprecating it, because it doesn't really harm in keeping it. It's not a lot of more code involved... So I don't mind in keeping it and don't deprecate it. We'll probably never change it in Fedora, so I think it's fine to keep it.

This needs to be expanded because supports_remote_rule is False by default (unless you create custom subject type in conf/subject_types/).

+1 :tada:

@gnaponie what do you think?

+1 Let's merge it! :)