Fixes #26050 - yum exclude list

[upadhyeammit]

This adds check for yum exclude list, if its configured then user gets error with message. The message includes which file has configured it and what is value of same.

[theforeman-bot]

Issues: #25939

[mbacovsky]

@upadhyeammit is the right issue linked to the PR. Shouldn't it be https://projects.theforeman.org/issues/26050?

[upadhyeammit]

@mbacovsky my mistake :( changed it ! Thank You !

[jameerpathan111]

@upadhyeammit I have tested this pr and found below issue.

• I have added exclude=kernel* abc* in /etc/yum.conf,
  the check fails as expected but the issue is that it doesn't show complete list.
  I think we should change grep -w exclude in such a way that we can get complete list.

--------------------------------------------------------------------------------
Check if yum exclude list is configured:                              [FAIL]
The /etc/yum.conf has exclude list configured as below,
  exclude = kernel*,
Unset this as it can cause yum update or upgrade failures !
--------------------------------------------------------------------------------

[upadhyeammit]

@jameerpathan111 I switched to yum.conf which is fixing your concern. Request to check.
Thank You !

[upadhyeammit]

@jameerpathan111 I can see one more issue with this pr, if yum.conf not having exclude defined it will fail ! The yum-config-manager use to help with this situation, I am checking this one further.

[iNecas]

What is the status here?

[upadhyeammit]

What is the status here?

From my side work is complete, when we had last meeting @mbacovsky mentioned that this may need changes because of version lock PR .

[iNecas]

@mbacovsky mind having a look?

[mbacovsky]

@upadhyeammit I did some more testing and it behaves almost as expected. However I took me quite some time to understand why. I've suggested some changes inside.

As for our discussion on using the package manager layer - I don't think we need to do that now. The functionality is nicely bundled in one file and I have no idea how excluding of packages works in Debian world so I guess it would slip this for too long. What do you think?

[mbacovsky]

I assumed from the exclude_set? name that it would return true if exclude is set in yum but it does the opposite. That confused me a lot. How about exclude_unset? or !exclude_used? or similar? Or did I miss something?

[upadhyeammit]

I changed it to return true when exclude is set, sorry for confusion !

[mbacovsky]

This call the grep twice in many cases and misses e.g. '# exclude=kernel*'. Could we use regexp instead like

yum_exclude =~ /^exclude\s*=\s*\S+/

[upadhyeammit]

I changed this to regular expression, miss from my side about commented exclude line.

[mbacovsky]

This feels quite hacky, could we return empty string here?
Perhaps we could call the method grep_yum_exclude to see better what it does.

[upadhyeammit]

I changed the method name and now using execute_with_status which returns empty string when exclude is not set.

[upadhyeammit]

@upadhyeammit I did some more testing and it behaves almost as expected. However I took me quite some time to understand why. I've suggested some changes inside.

As for our discussion on using the package manager layer - I don't think we need to do that now. The functionality is nicely bundled in one file and I have no idea how excluding of packages works in Debian world so I guess it would slip this for too long. What do you think?

I am also not familiar with how Debian handles excluding packages, I would explore on it but now what I feel is we should merge this for yum after taking care of suggestions added by you. I will check for Debian side and would be including it next.

[theforeman-bot]

There were the following issues with the commit message:

• bfc0cf1 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

---

This message was auto-generated by Foreman's prprocessor

[upadhyeammit]

@mbacovsky with recent commit only thing is we call grep twice when exclude is set, I was trying to avoid it but it was unnecessarily adding more lines to check and I feel one extra grep command call should not be that expensive. Request to review once ! thank you !

[mbacovsky]

@upadhyeammit thank you for the update and incorporating my comments. I checked on the PR and it looks good. However I'm still not very convinced about the unnecessary double grep call in the check (the execution is logged twice, new subshell is executed etc) so I'm sorry but I'd appreciate if we can get rid of it. Could you please reconsider it?

[upadhyeammit]

@upadhyeammit thank you for the update and incorporating my comments. I checked on the PR and it looks good. However I'm still not very convinced about the unnecessary double grep call in the check (the execution is logged twice, new subshell is executed etc) so I'm sorry but I'd appreciate if we can get rid of it. Could you please reconsider it?

No problem at all, will send fix for same ! thank you for review

[upadhyeammit]

@mbacovsky did changes as requested, thank you for help ! Request to have a look

[mbacovsky]

@upadhyeammit thank you for the update, I really appreciate it. I'll merge this once the tests finish and are green.

[upadhyeammit]

@upadhyeammit thank you for the update, I really appreciate it. I'll merge this once the tests finish and are green.

ack! Travis was failing due to freeze for regex, I removed it and it should be green now!

[mbacovsky]

Thanks again @upadhyeammit!