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

Remove PackageHub Activating code for w3m_https #10964

Merged
merged 1 commit into from Sep 10, 2020
Merged

Conversation

lilyeyes
Copy link
Contributor

@lilyeyes lilyeyes commented Sep 9, 2020

Remove PackageHub Activating code for fips test case "w3m_https".

Pkg "w3m" is not in "Packagehub" now (at lease on sle12sp5, sle15sp2) so delete the invoking of "setup_web_browser_env()" which activates "Packagehub",
"Packagehub" activation usually fails as it is not ready at the early developing stage (bug#1175578).
https://bugzilla.suse.com/show_bug.cgi?id=1175578

After the fix test case "w3m_https" is PASS now.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

I wonder why you need this change.
The commit message could have a bit more details, e.g. what issue is fixed, why this needs to change, to which versions of which product it applies, link to a bug or a fate entry, the choices you made, etc. Also see https://commit.style/ or http://chris.beams.io/posts/git-commit/ as a helpful guide how to write good commit messages. Also please rebase because there should be no "merge" commits in personal feature branches before merging into master.

I wonder what is the purpose of the zypper search line because this does not look like it has a direct relation to running w3m if the fips pattern exists or not. Also you are duplicating the code. Maybe that call is not necessary at all for w3m.

@lilyeyes
Copy link
Contributor Author

lilyeyes commented Sep 9, 2020

I wonder why you need this change.

Test case "w3m" failed on "Packagehub is not ready" but w3m pkg is not in packagehub now, so I deleted the "setup_web_browser_env" as it activates "Packagehub".
See the related poo for more info.

The commit message could have a bit more details, e.g. what issue is fixed, why this needs to change, to which versions of which product it applies, link to a bug or a fate entry, the choices you made, etc. Also see https://commit.style/ or http://chris.beams.io/posts/git-commit/ as a helpful guide how to write good commit messages. Also please rebase because there should be no "merge" commits in personal feature branches before merging into master.

Rebased. Thanks for the finding.
I left some message to the related poo, I think reviewer should read/understand the poo.
We also need to added them to PR, right?

I wonder what is the purpose of the zypper search line because this does not look like it has a direct relation to running w3m if the fips pattern exists or not. Also you are duplicating the code. Maybe that call is not necessary at all for w3m.

It makes sure that the "fips" pattern should be existed.

Thank you so much for you comments :)

@okurz
Copy link
Member

okurz commented Sep 9, 2020

I wonder why you need this change.

Test case "w3m" failed on "Packagehub is not ready" but w3m pkg is not in packagehub now, so I deleted the "setup_web_browser_env" as it activates "Packagehub".
See the related poo for more info.

Maybe the package hub activation code is still necessary for other versions of SLE then?

The commit message could have a bit more details, e.g. what issue is fixed, why this needs to change, to which versions of which product it applies, link to a bug or a fate entry, the choices you made, etc. Also see commit.style or http://chris.beams.io/posts/git-commit as a helpful guide how to write good commit messages. Also please rebase because there should be no "merge" commits in personal feature branches before merging into master.

Rebased. Thanks for the finding.
I left some message to the related poo, I think reviewer should read/understand the poo.
We also need to added them to PR, right?

I recommend you put more details in the git commit message as the git log should have enough information for anyone looking in the git history to understand why changes have been done and what needs to be kept in mind when further changes will be done in the future.

I wonder what is the purpose of the zypper search line because this does not look like it has a direct relation to running w3m if the fips pattern exists or not. Also you are duplicating the code. Maybe that call is not necessary at all for w3m.

It makes sure that the "fips" pattern should be existed.

Yes, I understand that. But why should a properly designed test for w3m care about the fips pattern? At least judging from the name of w3m that should focus on testing w3m and not care about a fips pattern.

@lilyeyes
Copy link
Contributor Author

Maybe the package hub activation code is still necessary for other versions of SLE then?

Totally agree with your concerns on this, I have tried to check on sle12sp5 but found no vm available or qcow in openQA now, also maintenance QA did not use/run test case w3m_https.
So, it is really time consuming so sometimes I prefer to fix the issue when introducing issue on other versions.
Also @jouyingbin helped to check the pkg w3m is in SLES12-SP5-Pool but not Packagehub now. Then there is no issue now.

The commit message could have a bit more details, e.g. what issue is fixed, why this needs to change, to which versions of which product it applies, link to a bug or a fate entry, the choices you made, etc. Also see commit.style or http://chris.beams.io/posts/git-commit as a helpful guide how to write good commit messages. Also please rebase because there should be no "merge" commits in personal feature branches before merging into master.

Got it. 👍

Rebased. Thanks for the finding.
I left some message to the related poo, I think reviewer should read/understand the poo.
We also need to added them to PR, right?

I recommend you put more details in the git commit message as the git log should have enough information for anyone looking in the git history to understand why changes have been done and what needs to be kept in mind when further changes will be done in the future.

Got it, thanks 👍

I wonder what is the purpose of the zypper search line because this does not look like it has a direct relation to running w3m if the fips pattern exists or not. Also you are duplicating the code. Maybe that call is not necessary at all for w3m.

It makes sure that the "fips" pattern should be existed.

Yes, I understand that. But why should a properly designed test for w3m care about the fips pattern? At least judging from the name of w3m that should focus on testing w3m and not care about a fips pattern.

Our test case name is "w3m_https" (verify w3m can access https servers with fips enabled on client)
But if without " fips enabled" this test case can be reused by others.

Pkg "w3m" is not in "Packagehub" now (at lease on sle12sp5, sle15sp2)
so delete the invoking of "setup_web_browser_env()" which activates
"Packagehub", "Packagehub" activation usually fails as it is not ready
at the early developing stage (see bug#1175578).
@lilyeyes lilyeyes changed the title Remove PackageHub Activating code for test w3m Remove PackageHub Activating code for w3m_https Sep 10, 2020
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

So, it is really time consuming

I am sorry if this was the case for you. I guess it could have been just as simple as:

if (is_sle('<=15-sp2')) {
    setup_web_browser_env();
}
else {
    zypper_call("--no-refresh --no-gpg-checks search -it pattern fips") if get_var('FIPS_ENABLED');
}

but if you think there is more to it, I could be wrong.

@okurz okurz merged commit d3a7d98 into os-autoinst:master Sep 10, 2020
@lilyeyes
Copy link
Contributor Author

So, it is really time consuming

I am sorry if this was the case for you. I guess it could have been just as simple as:

if (is_sle('<=15-sp2')) {
    setup_web_browser_env();
}
else {
    zypper_call("--no-refresh --no-gpg-checks search -it pattern fips") if get_var('FIPS_ENABLED');
}

but if you think there is more to it, I could be wrong.
Yes the code is similar/same. I mean I have to verify at lease on 15sp1 this code works but 15sp1 qcows are not easy to found in openQA or other ways.

Copy link
Contributor

@jouyingbin jouyingbin left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants