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
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@redhat.com 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@lists.fedorahosted.org To unsubscribe send an email to copr-devel-leave@lists.fedorahosted.org
On Tuesday, October 18, 2016 4:01:10 PM CEST Michal Novotny wrote:
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.
To make this general, you need `git url` and `lookaside cache` info. Now you have only `clone url`, which is neither general, nor Fedora specific (there's no need for clone url in case of fedpkg).
Pavel
clime
On Tue, Oct 18, 2016 at 11:44 AM, Pavel Raiskup praiskup@redhat.com 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@lists.fedorahosted.org To unsubscribe send an email to copr-devel-leave@lists.fedorahosted.org
copr-devel@lists.fedorahosted.org
-
Michal Novotny -
Pavel Raiskup