copr / copr

Clone

#352 [frontend] change repo ID format
Merged 5 years ago by msuchy. Opened 5 years ago by dturecek.
copr/ dturecek/copr copr_instances  into  master

file modified
+1 -1 
@@ -1,4 +1,4 @@ 
 

- [{{ copr_dir.repo_id }}]
 

+ [{{ repo_id }}]
 

  name=Copr repo for {{ copr_dir.name }} owned by {{ copr_dir.copr.owner_name }}
 

  baseurl={{ url | fix_url_https_backend }}
 

  type=rpm-md

file modified
+5 -1 
@@ -743,11 +743,15 @@ 
 

      if not mock_chroot:
 

          raise ObjectNotFound("Chroot {} does not exist".format(name_release))
 

  

+     repo_id = "copr:{0}:{1}:{2}".format(app.config["PUBLIC_COPR_HOSTNAME"],
clime commented 5 years ago

PUBLIC_COPR_HOSTNAME can contain port currently as well. It's e.g. used here: https://pagure.io/copr/copr/blob/master/f/frontend/coprs_frontend/coprs/templates/api.html#_37 and I typically put localhost:8080 there on my machine so that I get api config rendered correctly for my localhost instance running on 8080. Could you, please, as part of this PR provide another config value e.g. PUBLIC_COPR_NETLOC (https://docs.python.org/3/library/urllib.parse.html?highlight=urljoin#url-parsing) that would be used where the whole network location is expected to be? This means you should scan the code for PUBLIC_COPR_HOSTNAME string and replace that with PUBLIC_COPR_NETLOC where appropriate. You can also do it so that if PUBLIC_COPR_NETLOC is unset, you can set it to PUBLIC_COPR_HOSTNAME in frontend/coprs_frontend/coprs/__init__.py so that we have a fallback value for it.

clime commented 5 years ago

I think using _copr: prefix instead of justcopr: is slightly more suitable to recognize copr repos among the other ones and to inform users that this is a value they should not touch because it is being used by dnf copr plugin to recognize copr repos from the others. Not a big deal though.

Please don't delay this even more. I must say that we are getting to far in this PR, and in https://github.com/rpm-software-management/dnf-plugins-core/pull/253#discussion_r209524744 as well.

The repo_id as @dturecek proposes is unique enough to not collide across copr instances.

And if there are any other mostly theoretical doubts we should implement everything in dnf copr plugin, not here. That said -- dnf copr plugin should be responsible for generating the repo ID string based on the hub configuration file (and the repo ID we generate here should
be ignored). Anything alse is just work on some brittle magic.

That said -- please don't expect repo ID has any other property then uniqueness.

clime commented 5 years ago

Also have a look if we can't just use 'PUBLIC_COPR_BASE_URL' instead of introducing the new variable PUBLIC_COPR_NETLOC. Not needing to introduce a new variable would be better.

clime commented 5 years ago

Please don't delay this even more.

Adding or not adding _ is not really delaying anything.

Adding _ doesn't make any sense (except it would make the dnf output a bit more ugly). Touching the repo ID, which itself has no semantic makes no sense. -1. Again, repo ID has no semantics. If you wish to do something, please invent some aditional argument like this_is_a_copr_repo = True but not here in this pull request.

clime commented 5 years ago

Sorry but you are just making mess here and in the related PR as well. copr dnf repos are recognized by repo ID constant prefix. That prefix should ideally be _copr:. Please, stop commenting if you don't know exactly what you are saying.

As always, lack of experience and respect. I'm afraid the more arrogance we allow, the more @clime thinks is appropriate. All I can do here:
-1 for _ prefix
-1 for paying attention to repo ID value, it's place to put hacks

It's not place to put hacks. Sorry, I can not edit comments (pagure strikes again).

 
+                                         copr_dir.copr.owner_name.replace("@", "group_"),
 

+                                         copr_dir.name)
 

      url = os.path.join(copr_dir.repo_url, '') # adds trailing slash
 

      repo_url = generate_repo_url(mock_chroot, url)
 

      pubkey_url = urljoin(url, "pubkey.gpg")
 

      response = flask.make_response(
 

-         flask.render_template("coprs/copr_dir.repo", copr_dir=copr_dir, url=repo_url, pubkey_url=pubkey_url))
 

+         flask.render_template("coprs/copr_dir.repo", copr_dir=copr_dir, url=repo_url, pubkey_url=pubkey_url,
 

+                               repo_id=repo_id))
 

      response.mimetype = "text/plain"
 

      response.headers["Content-Disposition"] = \
 

          "filename={0}.repo".format(copr_dir.repo_name)

This PR changes the format of repo IDs to copr:<hub>:<user>:<project> so that it's clear from the ID to which Copr hub the repo file belongs. This change is in addition to the changes in the dnf plugin.

The whole thing in [] is repo_id. You shouldn't be calling repo_id just the last two elements.

Agreed with @clime.

Btw., could the copr: prefix be configurable (for other instances)?

Btw., could the copr: prefix be configurable (for other instances)?

For what purpose? Instances will differ in the :{{ hub }}: part, that should be enough.
I think, that it won't be easy to have configurable the copr: prefix because https://github.com/rpm-software-management/dnf-plugins-core/pull/253/ counts on it.

@frostyx now I get it, thanks!

rebased onto c790a503796f19c858fa2925ee3b7f0a08c539fb
5 years ago

I changed it so repo_id contains the whole ID, and rebased the branch to avoid conflicts.

+1, but please rather document the change in git commit message, instead of here in web UI.

I'd slightly prefer "copr:{0}:{1}:{2}".format(...)

rebased onto b0d8a2b
5 years ago

PUBLIC_COPR_HOSTNAME can contain port currently as well. It's e.g. used here: https://pagure.io/copr/copr/blob/master/f/frontend/coprs_frontend/coprs/templates/api.html#_37 and I typically put localhost:8080 there on my machine so that I get api config rendered correctly for my localhost instance running on 8080. Could you, please, as part of this PR provide another config value e.g. PUBLIC_COPR_NETLOC (https://docs.python.org/3/library/urllib.parse.html?highlight=urljoin#url-parsing) that would be used where the whole network location is expected to be? This means you should scan the code for PUBLIC_COPR_HOSTNAME string and replace that with PUBLIC_COPR_NETLOC where appropriate. You can also do it so that if PUBLIC_COPR_NETLOC is unset, you can set it to PUBLIC_COPR_HOSTNAME in frontend/coprs_frontend/coprs/__init__.py so that we have a fallback value for it.

I think using _copr: prefix instead of justcopr: is slightly more suitable to recognize copr repos among the other ones and to inform users that this is a value they should not touch because it is being used by dnf copr plugin to recognize copr repos from the others. Not a big deal though.

Please don't delay this even more. I must say that we are getting to far in this PR, and in https://github.com/rpm-software-management/dnf-plugins-core/pull/253#discussion_r209524744 as well.

The repo_id as @dturecek proposes is unique enough to not collide across copr instances.

And if there are any other mostly theoretical doubts we should implement everything in dnf copr plugin, not here. That said -- dnf copr plugin should be responsible for generating the repo ID string based on the hub configuration file (and the repo ID we generate here should
be ignored). Anything alse is just work on some brittle magic.

That said -- please don't expect repo ID has any other property then uniqueness.

Also have a look if we can't just use 'PUBLIC_COPR_BASE_URL' instead of introducing the new variable PUBLIC_COPR_NETLOC. Not needing to introduce a new variable would be better.

Please don't delay this even more.

Adding or not adding _ is not really delaying anything.

Adding _ doesn't make any sense (except it would make the dnf output a bit more ugly). Touching the repo ID, which itself has no semantic makes no sense. -1. Again, repo ID has no semantics. If you wish to do something, please invent some aditional argument like this_is_a_copr_repo = True but not here in this pull request.

Sorry but you are just making mess here and in the related PR as well. copr dnf repos are recognized by repo ID constant prefix. That prefix should ideally be _copr:. Please, stop commenting if you don't know exactly what you are saying.

Looks good to me, thanks.

As always, lack of experience and respect. I'm afraid the more arrogance we allow, the more @clime thinks is appropriate. All I can do here:
-1 for _ prefix
-1 for paying attention to repo ID value, it's place to put hacks

It's not place to put hacks. Sorry, I can not edit comments (pagure strikes again).

Pull-Request has been merged by msuchy
5 years ago
Metadata
None
No Tags
Changes Summary 2
+1 -1
file changed
frontend/coprs_frontend/coprs/templates/coprs/copr_dir.repo
+5 -1
file changed
frontend/coprs_frontend/coprs/views/coprs_ns/coprs_general.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.