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 lachmanfrantisek commented Mar 4, 2020

  • Split the JobTriggerType to one related to config and one for actual triggers.

@softwarefactory-project-zuul
Copy link
Contributor

softwarefactory-project-zuul bot commented Mar 4, 2020

Build succeeded.

@packit-as-a-service
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.

@softwarefactory-project-zuul
Copy link
Contributor

softwarefactory-project-zuul bot commented Mar 5, 2020

Build failed.

@lachmanfrantisek
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.)

@softwarefactory-project-zuul
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
@lachmanfrantisek lachmanfrantisek added the do-not-merge Work in progress. label Mar 6, 2020
@csomh
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
Member

@TomasTomecek TomasTomecek left a comment

lgtm, well done

packit/config/job_config.py Show resolved Hide resolved
@lachmanfrantisek
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
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
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/packit-service#453
csomh
csomh approved these changes Mar 6, 2020
@softwarefactory-project-zuul
Copy link
Contributor

softwarefactory-project-zuul bot commented Mar 6, 2020

Build succeeded.

@lachmanfrantisek lachmanfrantisek added mergeit When set, zuul wil gate and merge the PR. and removed do-not-merge Work in progress. labels Mar 6, 2020
@softwarefactory-project-zuul
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
Copy link
Member Author

lachmanfrantisek commented Mar 6, 2020

recheck

@TomasTomecek
Copy link
Member

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
Copy link
Contributor

softwarefactory-project-zuul bot commented Mar 6, 2020

Build succeeded.

@softwarefactory-project-zuul
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:master Mar 6, 2020
9 of 10 checks passed
@lachmanfrantisek lachmanfrantisek deleted the 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
Labels
mergeit When set, zuul wil gate and merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants