| Re: [review] small dist-git design issues

[Michal Novotny]

The code implements support for building from Fedora DistGit. The
subcommand of copr-cli is called `buildfedpkg` to suggest that.

Naming inside COPR code was chosen as such because the idea is to make
DistGit source general.

clime

On Tue, Oct 18, 2016 at 11:44 AM, Pavel Raiskup <praiskup(a)redhat.com&gt; wrote:

>  Thanks for the dist-git patch [1]!
>
>  There is an issue with naming, what does "dist-git" mean?  Is that copr's
>  dist-git?  Is that Fedora's dist-git?  Is that Red Hat Internal dist-git?
>
>  Can each package be built from different dist-git instance?  If not, why?
>
>  +class DistGitProvider(SrpmBuilderProvider):
>  +    def get_srpm(self):
>  +        cmd = ['git', 'clone', self.task.distgit_clone_url,
> self.tmp_dest]
>  +        output, error = VM.run(cmd, dst_dir=self.tmp_dest,
>  name=self.task.task_id)
>  +        log.info(output)
>  +
>  +        if self.task.distgit_branch:
>  +            cmd = ['fedpkg', '--dist', self.task.distgit_branch,
> 'srpm']
>  +        else:
>  +            cmd = ['fedpkg', 'srpm']
>
>  Please consider naming this Provider like FedoraDistGit, and rename
>  everything API related into Fedora namespace, because this is not generic
>  "dist-git".  If we have it "renamed", this would be fine contribution
> ...
>  but otherwise we waste namespace in api without a way back, and forever --
>  DON'T RELEASE YET.
>
>  Internally (RHEL) we also have dist-git, and it would be really nice to
>  have
>  this configurable so we can reuse this, or add RHELDistGit, too..
>
>  +        output, error = VM.run(cmd, dst_dir=self.tmp_dest,
>  name=self.task.task_id, cwd=self.tmp_dest)
>
>  Please consider moving all this expensive tasks onto builder.  That's the
>  only
>  _secure_ way to add "SRPM providers" and it is the only scalable way (koji
>  does
>  this right, we can possibly copy code from there - or at least study).
>  Also,
>  VM != virtualization in this case, which is confusing.
>
>  +        log.info(output)
>  +        self.copy()
>  ...
>
>  Seriously, this is basically good code, but start doing reviews so people
>  involved can discuss design issues.
>
>  [1] https://github.com/fedora-copr/copr/commit/308daed9235cb
>
>  Pavel
>
>  _______________________________________________
>  copr-devel mailing list -- copr-devel(a)lists.fedorahosted.org
>  To unsubscribe send an email to copr-devel-leave(a)lists.fedorahosted.org
>