PR#177: cleanup backend/get-build-task/<id>/ dict - copr/copr

copr / copr

#177 cleanup backend/get-build-task/<id>/ dict

Closed 6 years ago by praiskup. Opened 6 years ago by praiskup.

Unknown source get-build-task-cleanup into master

cleanup backend/get-build-task/<id>/ dict

Pavel Raiskup • 6 years ago

f6afc41

frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py

file modified

+1 -11

		`@@ -138,23 +138,13 @@`
		`"project_owner": task.build.copr.owner_name,`
		`"project_name": task.build.copr.name,`
		`"submitter": task.build.user.name if task.build.user else None, # there is no user for webhook builds`
		`- "chroot": task.mock_chroot.name,`

		`- "repos": task.build.repos,`
		`"memory_reqs": task.build.memory_reqs,`
		`"timeout": task.build.timeout,`
		`"enable_net": task.build.enable_net,`
		`"git_repo": task.build.package.dist_git_repo,`
		`"git_hash": task.git_hash,`
		`- "source_type": helpers.BuildSourceEnum("scm"),`
		`- "source_json": json.dumps(`
		`- {'clone_url': task.build.package.dist_git_clone_url, 'committish': task.git_hash}),`
		`-`
		`- "package_name": task.build.package.name,`
		`- "package_version": task.build.pkg_version,`
		`- "repos": build_config.get("repos"),`
		`- "buildroot_pkgs": build_config.get("additional_packages"),`
		`- "use_bootstrap_container": build_config.get("use_bootstrap_container")`
		`+ "build_config": build_config,`
		`}`

		`except Exception as err:`

praiskup commented 6 years ago

This is for discussion purposes only.. (not intended to be merged!).

From logical POV, it seems to me that we could cleanup the build dict for copr-rpmbuild
this way, to simplify the code and to keep this in sync with api/.../build-config/.../ page.
If we can do this, I would follow up with more work done, removing a some duplication
of code/interests in copr mock-config and copr-rpmbuild ...

Copying the whole build_config seems to be clear plus to me, just the consumer needs
to be adjusted a bit... though what I'm really worried about is these fields:

source_type
source_json
package_name
package_version

At this point the package is imported in dist-git completely, right? Why we need
them then?

clime commented 6 years ago

source_type and source_json are pretty much the essential information describing where to get the sources from. Currently we rely on what has been imported into our dist-git and send SCM build request to backend to build the package from our DistGit. That might change in future.

package_name and package_version are not really necessary to be there I think but the purpose of these fields is to give users information about the package that has been imported into our dist-git. These fields can probably be removed from the job specification as they have only informational value for users.

Also I would suggest removing git_repo and git_hash fields that are kept there for backward compatibility with the original builder script. I though the idea for a long time is that the original builder script is deprecated?

msuchy commented 6 years ago

memory_reqs can be wiped too. And from client. It is not used.

praiskup commented 6 years ago

source_type and source_json are pretty much the essential information describing where to get the sources from. Currently we rely on what has been imported into our dist-git and send SCM build request to backend to build the package from our DistGit. That might change in future.

That's IMO the "kill-dist-git" => "implement proxy" idea, which I still believe is not the future. Let's talk about this f2c today..

package_name and package_version are not really necessary to be there I think but the purpose of these fields is to give users information about the package that has been imported into our dist-git. These fields can probably be removed from the job specification as they have only informational value for users.

Ack, I would remove that -> since the dist-git content (clone url + commit) should be 100% self-explaining.

praiskup commented 6 years ago

Also I would suggest removing git_repo and git_hash fields that are kept there for backward compatibility with the original builder script. I though the idea for a long time is that the original builder script is deprecated?

Ah, how do I know how to clone from dist-git then?

clime commented 6 years ago

Also I would suggest removing git_repo and git_hash fields that are kept there for backward compatibility with the original builder script. I though the idea for a long time is that the original builder script is deprecated?

Ah, how do I know how to clone from dist-git then?

The information is in source_json field.

clime commented 6 years ago

package_name and package_version are not really necessary to be there

Actually, backend uses this information for name of the target build directory (package_name) and for build messages (package_version).

clime commented 6 years ago

Sorry for wrong info.

clime commented 6 years ago

Is there going to be any more progress here?

praiskup commented 6 years ago

Not necessarily here in this PR, thank you.

Pull-Request has been closed by praiskup

6 years ago

praiskup commented 6 years ago

Actually, backend uses this information for name of the target build directory (package_name) and for build messages (package_version).

Reading again, how backend reads this generated json?

clime commented 6 years ago

Actually, backend uses this information for name of the target build directory (package_name) and for build messages (package_version).

Reading again, how backend reads this generated json?

You mean where it accesses package_name and package_version fields? It's in class BuildJob in backend/job.py.

praiskup commented 6 years ago

Ah, but that config dict having package_name comes from BuildDispatcher.load_job() (api for backend), not really from the /backend/get-build-task/ json dict (api for builder). So it seems like dropping it here makes sense.

clime commented 6 years ago

Hmm, you are dropping it in get_build_record and that method is called from def waiting as well as def get_build_task.

praiskup commented 6 years ago

Meh meh. I see now, thanks for explanation. But this all is IMO unnecessarily misleading; what if we dropped (a) the unnecessary fields for backend from /waiting/ dict, and (b) unnecessarily fields for rpmbuilder from /get-build-task/? Backend doesn't need the enable_net, memory_reqs, timeout (once we drop the copr-builder support from backend) ..., and then the BuildJob deserves huge cleanup too (e.g. the pkg_version @property can be dropped, which means we can drop the pkg_main_version, etc., etc..).

clime commented 6 years ago

Meh meh. I see now, thanks for explanation. But this all is IMO unnecessarily misleading; what if we dropped (a) the unnecessary fields for backend from /waiting/ dict, and (b) unnecessarily fields for rpmbuilder from /get-build-task/? Backend doesn't need the enable_net, memory_reqs, timeout (once we drop the copr-builder support from backend) ..., and then the BuildJob deserves huge cleanup too (e.g. the pkg_version @property can be dropped, which means we can drop the pkg_main_version, etc., etc..).

I agree about the cleanup needed.