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.

Sign up for GitHub

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

Jump to bottom

Feature/debian errata working #9886

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Feature/debian errata working #9886

wants to merge 1 commit into from
+380 −20

Conversation

mbgonicus
Copy link
Contributor

What are the changes introduced in this pull request?

It is basically PR #7961 but with some changes to make it work with foreman 4.3.0 rc4.

It introduces a table column nva for tables katello_debs and katello_erratum_deb_packages to calculate the errata for a package more quickly and easily. The rest are minor adjustments.

Considerations taken when implementing this change?

The changes are made for a specific customer of our company. So the changes might not be completely suitable for the merge. But we thought we share our changes to help the progress with the debian errata feature.

What are the testing steps for this pull request?

We did manual testing on several real systems. The contract does not have budget for automated testing in katello, sorry.

@theforeman-bot
Copy link

Can one of the admins verify this patch?

@theforeman-bot
Copy link

Issues: #33838

@ekohl
Copy link
Member

ekohl commented Jan 14, 2022

Looks like your branch includes way too many commits. Could you rebase this on master?

@mbgonicus
A working debian errata feature
fc98d36 
This commit is based on PR Katello#7961 and introduces some changes to make
it work on forman 4.3.0 rc4.

Refs #25978
@mbgonicus mbgonicus force-pushed the feature/debian_errata_working branch from ba6675b to fc98d36 Compare January 14, 2022 12:03
@mbgonicus
Copy link
Contributor Author

Looks like your branch includes way too many commits. Could you rebase this on master?

Sorry, it was based on 4.3.0 RC 4. I force-pushed a rebase.

@jturel
Copy link
Member

jturel commented Jan 14, 2022

[test katello]

@melcorr melcorr mentioned this pull request Feb 18, 2022
Open
@chris1984 chris1984 closed this May 18, 2022
@melcorr
Copy link

melcorr commented Jun 8, 2022

@chris1984 why was this closed?

@chris1984
Copy link
Member

@melcorr I think I closed it since there has not been any updates and I was closing out some stale prs. The user can reopen it if they want.

@mbgonicus
Copy link
Contributor Author

@chris1984 What updates did you expect? Since I opened this PR, I received pretty much no feedback...

@melcorr
Copy link

melcorr commented Jun 8, 2022

@mbgonicus - do you consider this PR complete? What do you need? Reviews?

@mbgonicus
Copy link
Contributor Author

@melcorr Well, we have that code working and are using it in production.

Basically, you need to review whether this code is suitable for your standards and if you want to include it into katello.

@melcorr
Copy link

melcorr commented Jun 8, 2022

@mbgonicus can you reopen, please?

@mbgonicus
Copy link
Contributor Author

@melcorr It seems, I cannot - probably no permission

@melcorr
Copy link

melcorr commented Jun 8, 2022

@mbgonicus I'll get someone to

@melcorr
Copy link

melcorr commented Jun 8, 2022

@chris1984 can you open this PR again?
Since I've been involved in the Foreman community, people have been asking for this.
I would appreciate if Katello folks could review. This is potentially a very valuable contribution.

@chris1984 chris1984 reopened this Jun 8, 2022
@chris1984
Copy link
Member

@melcorr reopened and can take a look and add it to the Jira board to review. @mbgonicus can you rebase so we can start the review process. Sorry we missed it.

@lumarel
Copy link
Contributor

lumarel commented Jun 8, 2022

I'm also really looking forward to the deb Errata feature.
But sorry for asking, what was the motivation behind improving the changes from the ATIX team and creating a separate PR (with a single commit) instead of getting in touch with them and integrating the changes in the already present PR?

I have been following the other PR since a while now, and it looks like there is still work done. I'm not really saying this is a bad thing, but maybe it would help to get to the best solution with joint forces.

@mbgonicus
Copy link
Contributor Author

I'm also really looking forward to the deb Errata feature. But sorry for asking, what was the motivation behind improving the changes from the ATIX team and creating a separate PR (with a single commit) instead of getting in touch with them and integrating the changes in the already present PR?

I have been following the other PR since a while now, and it looks like there is still work done. I'm not really saying this is a bad thing, but maybe it would help to get to the best solution with joint forces.

Well we had that deb errata as a customer request. Their system was a little special due to customizations and the other PR was not finished. Because it had to be done quickly (within 2 days) we just hacked our way through. But since it is working, we just wanted to give the code to you, hoping it might help.

So if you want to modify this code or just pick some stuff to the other PR, please feel free to do so.

@chris1984 I'll try to find some time to rebase it

@lumarel
Copy link
Contributor

lumarel commented Jun 9, 2022 •
edited
Loading

I'm also really looking forward to the deb Errata feature. But sorry for asking, what was the motivation behind improving the changes from the ATIX team and creating a separate PR (with a single commit) instead of getting in touch with them and integrating the changes in the already present PR?
I have been following the other PR since a while now, and it looks like there is still work done. I'm not really saying this is a bad thing, but maybe it would help to get to the best solution with joint forces.

Well we had that deb errata as a customer request. Their system was a little special due to customizations and the other PR was not finished. Because it had to be done quickly (within 2 days) we just hacked our way through. But since it is working, we just wanted to give the code to you, hoping it might help.

So if you want to modify this code or just pick some stuff to the other PR, please feel free to do so.

@chris1984 I'll try to find some time to rebase it

Yes exactly, and everything I meant with that message was, why it wasn't reached out to the team who already has the PR running and asking them if they want the additional changes, and integrate them in their PR (which is possible to send an PR against the PR in the ATIX repo)
Because they will most likely also be the once who will improve and further manage that part if I look in the history of the commits.

(I'm just a user not one of their team, sorry if it looked like that)

@melcorr
Copy link

melcorr commented Jun 9, 2022

@lumarel over the last two years, I have been annoying @sbernhard for updates on the other PR. I think that the migration to Pulp 3 ate into a lot of that team's time and resources. I don't think that there was capacity for this, or at least that was what was reported. @quba42 gave a roadmap update at our birthday celebration last year and I'm pretty sure he reported the same. I hope that ATIX can build on this if we get it merged.

@lumarel
Copy link
Contributor

lumarel commented Jun 9, 2022

@melcorr It's definitely not an easy task to integrate all these functions, as far as I have seen that already included like 5 PRs in 4 projects (or even more), so I'm really grateful that they are taking so much time to upstream their effort! :)

It's just that I can see like 3 commits from a months ago, which are not part of this one (additionally to this being a single commit with the content of some of the commits of the other PR).
And maybe some of the additional stuff here would make it possible to merge the other PR quite soon, because @mbgonicus already thought about the parts which were not polished in the other one!

@martux69
Copy link

Are there any news on this? Perhaps a release date?

@mbgonicus
Copy link
Contributor Author

I have trouble solving the merge conflicts. I never understood a lot of this project and cannot judge what would be correct.

However, I reckon we need someone with in-deep knowledge to have a look into this and the other PR who can evaluate and sum up the "correct" way of implementing this feature.

@shay1197
Copy link

Hi guys any update?? if its works or not

@martux69
Copy link

Could someone post a status update for this issue? Perhaps is a final release date at the horizon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fsolen fsolen fsolen approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Needs testing Not yet reviewed Waiting on contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@mbgonicus @theforeman-bot @ekohl @jturel @melcorr @chris1984 @lumarel @martux69 @shay1197 @fsolen