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

Nailgun Updates for Needs_publish feature #922

Merged
merged 3 commits into from
Jun 28, 2023
Merged

Nailgun Updates for Needs_publish feature #922

merged 3 commits into from
Jun 28, 2023

Conversation

sambible
Copy link
Contributor

@sambible sambible commented May 8, 2023

Simple PR, adds a new flag for the content-view publish audit feature.

@sambible sambible added the No-CherryPick PR doesnt need CherryPick to previous branches label May 8, 2023
@sambible sambible mentioned this pull request May 8, 2023
Merged
@sambible sambible force-pushed the content-view-audit branch from c6f8692 to 66b3c69 Compare May 8, 2023 18:57
@codecov
Copy link

codecov bot commented May 8, 2023 •
edited
Loading

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (33db04b) 92.25% compared to head (109f12c) 92.25%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #922      +/-   ##
==========================================
- Coverage   92.25%   92.25%   -0.01%     
==========================================
  Files           6        6              
  Lines        3073     3071       -2     
==========================================
- Hits         2835     2833       -2     
  Misses        238      238
Impacted Files Coverage Δ
nailgun/entities.py 91.32% <ø> (-0.01%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sambible
Copy link
Contributor Author

sambible commented May 8, 2023

This will be exercised via SatelliteQE/robottelo#11433

@sambible sambible changed the title Add needs_publish flag Nailgun Updates for Needs_publish feature May 16, 2023
@sambible
Copy link
Contributor Author

Added another nailgun change here for the needs_publish feature, figured it didn't make sense to make another PR

ogajduse
ogajduse reviewed May 19, 2023
View reviewed changes
Copy link
Member

@ogajduse ogajduse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending question. LGTM

nailgun/entities.py Show resolved Hide resolved
@jyejare jyejare force-pushed the content-view-audit branch from 674b10c to d73e01a Compare June 14, 2023 07:24
jyejare
jyejare requested changes Jun 14, 2023
View reviewed changes
Copy link
Member

@jyejare jyejare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK pending Cherrypick question from @ogajduse !

@sambible
Copy link
Contributor Author

Sorry for late replies here, forgot I had a nailgun PR here. I'll address the comments.

@sambible sambible force-pushed the content-view-audit branch from d73e01a to e43b6a6 Compare June 20, 2023 18:02
@sambible
Copy link
Contributor Author

Alright, so with respect to the API for components changing: It hasn't. This portion that I removed from the Nailgun entity seems to be only something within Nailgun, rather than anything within satellite. This entity doesn't have any tests utilizing it currently, and I can't find anywhere there were tests. So I think this is just something that's a nailgun cleanup, and should be backported to any version of Satellite we care about writing tests retroactively for.

@ogajduse
Copy link
Member

ogajduse commented Jun 20, 2023 •
edited
Loading

Alright, so with respect to the API for components changing: It hasn't. This portion that I removed from the Nailgun entity seems to be only something within Nailgun, rather than anything within satellite. This entity doesn't have any tests utilizing it currently, and I can't find anywhere there were tests. So I think this is just something that's a nailgun cleanup, and should be backported to any version of Satellite we care about writing tests retroactively for.

Thanks for looking into it. Funny that #920 (open at the moment) makes use of the import that you are removing.
Can we cherry-pick this patch to the older branches as-is now? If yes, then let's do 6.11+

Gauravtalreja1
Gauravtalreja1 previously approved these changes Jun 21, 2023
View reviewed changes
Copy link
Collaborator

@Gauravtalreja1 Gauravtalreja1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, pending comment for cherrypick @ogajduse and add respective branch labels with Cherrypick label

@Satellite-QE Satellite-QE mentioned this pull request Jun 21, 2023
Merged
@sambible sambible force-pushed the content-view-audit branch from e43b6a6 to 7dc04f5 Compare June 22, 2023 15:31
@sambible
Copy link
Contributor Author

Will need to create a seperate cherrypick, since those older versions won't have the needs_publish feature - I'll create a github issue and link it here to capture that effort

@sambible sambible mentioned this pull request Jun 22, 2023
Open
@sambible
Copy link
Contributor Author

#943

ogajduse
ogajduse approved these changes Jun 23, 2023
View reviewed changes
Copy link
Member

@ogajduse ogajduse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Can you re-review @Gauravtalreja1?

@ogajduse
Copy link
Member

Shouldn't we cherry-pick this PR to 6.14, @sambible?

@sambible sambible added CherryPick PR needs CherryPick to previous branches 6.14.z and removed No-CherryPick PR doesnt need CherryPick to previous branches labels Jun 26, 2023
@sambible
Copy link
Contributor Author

We definitely need to, thanks for the catch @ogajduse - this PR has been up for so long, I had hoped it would be in before branching haha.

@sambible sambible force-pushed the content-view-audit branch from baafc07 to 109f12c Compare June 26, 2023 13:53
@sambible
Copy link
Contributor Author

@jyejare can you merge this today please? It is ready to go :)

@vsedmik vsedmik requested a review from jyejare June 28, 2023 08:50
@jyejare jyejare merged commit 46e937b into SatelliteQE:master Jun 28, 2023
@Satellite-QE Satellite-QE mentioned this pull request Jun 28, 2023
Merged
github-actions bot pushed a commit that referenced this pull request Jun 28, 2023
@sambible @web-flow
Nailgun Updates for Needs_publish feature (#922)
5afea04 
* Add needs_publish flag

* Fix ContentViewComponent add method

* Readd payload mixin

(cherry picked from commit 46e937b)
sambible added a commit that referenced this pull request Jun 28, 2023
@sambible
Nailgun Updates for Needs_publish feature (#922)
a1cb059 
* Add needs_publish flag

* Fix ContentViewComponent add method

* Readd payload mixin

(cherry picked from commit 46e937b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsedmik vsedmik vsedmik approved these changes

@Gauravtalreja1 Gauravtalreja1 Gauravtalreja1 approved these changes

@ogajduse ogajduse ogajduse approved these changes

@jyejare jyejare jyejare approved these changes

Assignees

@sambible sambible

Labels
6.14.z CherryPick PR needs CherryPick to previous branches
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sambible @ogajduse @jyejare @Gauravtalreja1 @vsedmik