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

Media upgrade does not need test deregister #4947

Merged
merged 1 commit into from May 7, 2018
Merged

Conversation

dehai
Copy link
Contributor

@dehai dehai commented Apr 26, 2018

SLE15 does not have tcm anymore,it has integrated into development tools module.But, in this test module, it uses tcm as certain module to test this feature. It has not suitable for SLE15 testing and we do not need to test this module in migration job group.

@@ -1110,7 +1110,7 @@ sub load_consoletests {
}
}
loadtest 'console/install_all_from_repository' if get_var('INSTALL_ALL_REPO');
if (check_var_array('SCC_ADDONS', 'tcm') && get_var('PATTERNS') && sle_version_at_least('12-SP3')) {
if (check_var_array('SCC_ADDONS', 'tcm') && get_var('PATTERNS') && is_sle('<15') && !get_var("MEDIA_UPGRADE")) {
Copy link
Member

Choose a reason for hiding this comment

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

with this PR you also exclude deregister from SLE 15 and add it for all other SLE versions lower SLE 15 . Not say that it is wrong per ce but can you explain your motivation ?

Copy link
Member

Choose a reason for hiding this comment

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

@dehai I think you have read the sle_version_at_least('12-SP3') the wrong way around. Should be is_sle('12-SP3+'), isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@okurz @asmorodskyi because SLE15 does not have tool chain module, it has integrated into development tools module.

Copy link
Member

Choose a reason for hiding this comment

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

@dehai and what about SLE 12 SP1 and SLE 12 SP2 ? are you ok to deregister tcm there ?

Copy link
Contributor

@Jiawei517 Jiawei517 May 2, 2018

Choose a reason for hiding this comment

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

@dehai, I don't think it is reasonable solution here. We don't have tcm in SLE15, but we do have deregister function in SLE15. In this case, we would never run SUSEConnect -d to dergister modules in SLE15 again.

Copy link
Member

Choose a reason for hiding this comment

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

Please put something into the code. The code should be self a explanatory, if not at least describe the reason in a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@okurz added reason on comment.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I don't see any updated source code which would include the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@okurz I have rebased my PR, and updated reasons on commit. I don't add this long explanatory into code because this is main_common module, if added, it will be very ugly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dehai I think you do better add the comment to lib/main_common.pm

@dehai
Copy link
Contributor Author

dehai commented May 4, 2018

@okurz As it has blocked some migration jobs, could you merge it if you don't have any concerns. Thanks!!

SLE15 does not have tcm anymore, it has integrated
into development tools module. But in this test module
it uses tcm as certain module to test this feature.
It has not suitable for SLE15 testing and we do not
need to test this module in migration job group.
@okurz okurz merged commit 99201be into os-autoinst:master May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants