Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split job and jobconfig (packit) #746

Conversation

@lachmanfrantisek
Copy link
Member

lachmanfrantisek commented Mar 4, 2020

  • Split the JobTriggerType to one related to config and one for actual triggers.
@lachmanfrantisek lachmanfrantisek mentioned this pull request Mar 4, 2020
3 of 3 tasks complete
@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Mar 4, 2020

Build succeeded.

@packit-as-a-service

This comment has been minimized.

Copy link

packit-as-a-service bot commented Mar 4, 2020

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/packit-service-packit-746
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@lachmanfrantisek lachmanfrantisek force-pushed the lachmanfrantisek:split-job-and-jobconfig branch from 9b2daeb to ad5fc35 Mar 5, 2020
@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Mar 5, 2020

Build failed.

@lachmanfrantisek

This comment has been minimized.

Copy link
Member Author

lachmanfrantisek commented Mar 6, 2020

Do not merge this before we are ready in the p-s pull-request otherwise, the p-s code will be broken. (Ideally, merge this one, retest the p-s one and merge it in a short period.)

@lachmanfrantisek lachmanfrantisek force-pushed the lachmanfrantisek:split-job-and-jobconfig branch from ad5fc35 to 67f910a Mar 6, 2020
@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Mar 6, 2020

Build succeeded.

@lachmanfrantisek lachmanfrantisek changed the title WIP: Split job and jobconfig (packit) Split job and jobconfig (packit) Mar 6, 2020
@csomh

This comment has been minimized.

Copy link
Member

csomh commented Mar 6, 2020

I'm having difficulties understanding the context of this change: why is it needed, and why is it done the way it is?

Could you capture some of these details in the commit message. (See this explanation, why this is nice to have).

Copy link
Contributor

TomasTomecek left a comment

lgtm, well done

packit/config/job_config.py Show resolved Hide resolved
@lachmanfrantisek

This comment has been minimized.

Copy link
Member Author

lachmanfrantisek commented Mar 6, 2020

Could you capture some of these details in the commit message. (See this explanation, why this is nice to have).

I don't want to have it in the commit since it's cross-project change and complex and I am not sure if it will be useful to have such long message in the commit.

The context (which I wanted to discuss on the arch-meeting):

  • By now, we have JobTriggerType which is used in the config as well as for the events we are using in the packit-service.
  • Some triggers cannot be configured by users (e.g. CoprBuildEnd,..)
  • Problem 1: We are using one enum for multiple purposes.
  • Problem 2: It's hard to get the right config for the "fake" triggers.
  • Problem 3: Current workflow does not support multiple triggers for one action.
  • Here is a first part of the solution to the problems -- split the enum to two.

(See this

Nice article, I know about it.

@csomh

This comment has been minimized.

Copy link
Member

csomh commented Mar 6, 2020

Could you capture some of these details in the commit message. (See this explanation, why this is nice to have).

I don't want to have it in the commit since it's cross-project change and complex and I am not sure if it will be useful to have such long message in the commit.

These details should be captured in the commit message especially because there are complex and cross-project.

As for the usefulness:

  1. we would have saved a lot of typing time during this review, if this information would have been in the commit message (I was expecting it and looking for it) 😃
  2. could have helped folks not aware of this change coming up (me 😕 ) to have a meaningful contribution to the review process while also catching up with what's going on.
  3. will potentially save a lot of time for someone in the future, trying to re-establish the context of this change in order to fix some issue.
  4. good commit messages will help us immensely with building a community of contributors around Packit, as it will be much easier for outside contributors to understand how the project evolved, without us having to spend super much extra effort to keep some separate set of docs up to date and relevant.
  5. the folks writing the blog posts on Fridays wouldn't need to spend hours reading through all the diffs of all the changes during the week trying to figure out why things happened, so that they can summarise it.

The context (which I wanted to discuss on the arch-meeting):

  • By now, we have JobTriggerType which is used in the config as well as for the events we are using in the packit-service.
  • Some triggers cannot be configured by users (e.g. CoprBuildEnd,..)
  • Problem 1: We are using one enum for multiple purposes.
  • Problem 2: It's hard to get the right config for the "fake" triggers.
  • Problem 3: Current workflow does not support multiple triggers for one action.
  • Here is a first part of the solution to the problems -- split the enum to two.

Very nice explanation, thanks!

(See this

Nice article, I know about it.

@lachmanfrantisek

This comment has been minimized.

Copy link
Member Author

lachmanfrantisek commented Mar 6, 2020

These details should be captured in the commit message especially because there are complex and cross-project.

complex +1, cross-project -1

I will add the content of the comment to the git message. (edit: done)

…triggers

Signed-off-by: Frantisek Lachman <flachman@redhat.com>

By now, we have JobTriggerType which is used in the config as well as for the events we are using in the packit-service.

- Some triggers cannot be configured by users (e.g. CoprBuildEnd,..)
- Problem 1: We are using one enum for multiple purposes.
- Problem 2: It's hard to get the right config for the "fake" triggers.
- Problem 3: Current workflow does not support multiple triggers for one action.
- Here is a first part of the solution to the problems -- split the enum to two.
- Related PR in the packit-service repo: packit-service/packit-service#453
@lachmanfrantisek lachmanfrantisek force-pushed the lachmanfrantisek:split-job-and-jobconfig branch from 67f910a to 2714a92 Mar 6, 2020
@csomh
csomh approved these changes Mar 6, 2020
@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Mar 6, 2020

Build succeeded.

@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Mar 6, 2020

Build failed (gate pipeline). For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@lachmanfrantisek

This comment has been minimized.

Copy link
Member Author

lachmanfrantisek commented Mar 6, 2020

recheck

@TomasTomecek

This comment has been minimized.

Copy link
Contributor

TomasTomecek commented Mar 6, 2020

I'd say it's either a commit message or somewhere in the PR, but I agree with @csomh that it should be written down for the reviewers

well described and explained 🗻

for the Fiday fun, we should a tool to pre-generate the changelog - since I'm doing it today I may give the script a go

@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Mar 6, 2020

Build succeeded.

@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Mar 6, 2020

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 5fcddeb into packit-service:master Mar 6, 2020
9 of 10 checks passed
9 of 10 checks passed
packit/rpm-build-fedora-30-x86_64 RPM build has started...
Details
LGTM analysis: Python No new or fixed alerts
Details
ci/dockercloud Your tests passed in Docker Cloud
Details
local/check check status: success
Details
local/gate gate status: success
Details
packit/rpm-build-fedora-31-x86_64 RPMs were built successfully.
Details
packit/rpm-build-fedora-32-x86_64 RPMs were built successfully.
Details
packit/rpm-build-fedora-rawhide-x86_64 RPMs were built successfully.
Details
packit/testing-farm-fedora-30-x86_64 All tests passed
Details
packit/testing-farm-fedora-31-x86_64 All tests passed
Details
@lachmanfrantisek lachmanfrantisek deleted the lachmanfrantisek:split-job-and-jobconfig branch Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.