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

Remove long deprecated entities and Version based logic blocks #767

Merged
merged 1 commit into from
Dec 22, 2020
Merged

Remove long deprecated entities and Version based logic blocks #767

merged 1 commit into from
Dec 22, 2020

Conversation

mshriver
Copy link
Member

@mshriver mshriver commented Nov 6, 2020 •
edited
Loading

Removed:

  • Deprecated System and SystemPackage entities
  • Entity method workarounds based on old versions of Satellite (6.1 and 6.2)
  • server_modes
  • Unit tests related to all of the above

Test Results

/view/Satellite 6/job/satellite6-standalone-automation/2089/testReport/

  • tests.foreman.api.test_contentview.TestContentViewRedHatContent Has some failures in the standalone in test setup, a 403 on access to katello/api/v2/repository_sets, I don't believe this is related to my changes.
  • Additional failures are in unmodified classes, I have confidence this PR doesn't have any negative impact on test results in robottelo. I'll followup with fixes for anything that does see an impact, but the test results give me confidence in the change.

@mshriver mshriver mentioned this pull request Nov 6, 2020
Merged
JacobCallahan
JacobCallahan requested changes Nov 9, 2020
View reviewed changes
Copy link
Member

@JacobCallahan JacobCallahan left a comment

Choose a reason for hiding this comment

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

Just as a reminder for this PR, references to "sat" and "sam" as server_modes should be removed as well.

@mshriver mshriver force-pushed the remove-ancient-artifacts branch 4 times, most recently from 09a66ad to c8944ae Compare November 13, 2020 20:52
@mshriver mshriver requested review from JacobCallahan and a team November 13, 2020 20:55
@coveralls
Copy link

coveralls commented Nov 13, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.04%) to 96.953% when pulling 1a095b6 on mshriver:remove-ancient-artifacts into 20f2307 on SatelliteQE:master.

@mshriver mshriver marked this pull request as ready for review November 13, 2020 21:56
@JacobCallahan
Copy link
Member

Could you include the results of a standalone automation run for say activation keys?

mshriver added a commit to mshriver/robottelo that referenced this pull request Nov 17, 2020
@mshriver
Set requirements to use mshriver/nailgun#remove-ancient-artifacts
90a0281 
For testing this branch:
SatelliteQE/nailgun#767
swadeley
swadeley reviewed Nov 18, 2020
View reviewed changes
Copy link
Contributor

@swadeley swadeley left a comment

Choose a reason for hiding this comment

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

Have you tested with just removing all the 6.1 related code?

tests/test_entities.py Outdated
@@ -69,7 +70,7 @@ class InitTestCase(TestCase):
def setUpClass(cls):
"""Set a server configuration at ``cls.cfg``."""
cls.cfg = config.ServerConfig('http://example.com')
cls.cfg_610 = config.ServerConfig('http://example.com', version='6.1.0')
cls.cfg_610 = config.ServerConfig('http://example.com', version='6.8.0')
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why you have cls.cfg_610 not 680 here. I further wonder why you cannot just remove all the 6.1 stuff. Satellite 6.1 was quite different, but we do not need that code in master.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just missed updating the name here.

As you'll see I have removed entire tests for some entities, but that was focused on which unit tests started failing, and whether or not they were still relevant. I'll take a second look at how relevant this test is and at a minimum update the name to be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some removals may take some input from component owners, or a whole lot of investigation by me - we've got entity mixin function overrides that per their docblocks are to workaround BZ's that were written and closed years ago.

swadeley
swadeley previously approved these changes Nov 30, 2020
View reviewed changes
Copy link
Contributor

@swadeley swadeley left a comment

Choose a reason for hiding this comment

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

ACK

jyejare
jyejare previously approved these changes Dec 8, 2020
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 with non blocking improvements.

nailgun/entity_mixins.py Outdated Show resolved Hide resolved
nailgun/entity_mixins.py Outdated Show resolved Hide resolved
tests/test_entities.py Outdated Show resolved Hide resolved
@mshriver mshriver dismissed stale reviews from jyejare and swadeley via dbb37e6 December 21, 2020 14:27
@mshriver
Remove long deprecated entities and Version based logic blocks
1a095b6 
Version logic blocks based on Sat 6.1 and 6.2, as well as long deprecated System and SystemPackge entities removed

Eliminate server_modes
no longer valid for katello/satellite

Update and remove unit tests
@jyejare jyejare merged commit 2db9328 into SatelliteQE:master Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JacobCallahan JacobCallahan JacobCallahan approved these changes

@jyejare jyejare jyejare approved these changes

@swadeley swadeley swadeley left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mshriver @coveralls @JacobCallahan @swadeley @jyejare