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

Fix cached osrelease_info grain type #55796

Merged
merged 1 commit into from Jan 13, 2020

Conversation

srg91
Copy link
Contributor

@srg91 srg91 commented Jan 6, 2020

What does this PR do?

This pull-request fixes problems with operating on osrelease_info grain if it was loaded from cache. Now it loads correctly as tuple.

What issues does this PR fix or reference?

I found the one: #54908

Previous Behavior

loader.py loads grain osrelease_info as list.
It broke modules like aptpkg.info_installed and similar.

New Behavior

Now osrelease_info loads as tuple

Tests written?

Yes, unit.test_loader.LoaderLoadCachedGrainsTest

Commits signed with GPG?

No

@srg91 srg91 requested a review from as a code owner Jan 6, 2020
@ghost ghost requested a review from garethgreenaway Jan 6, 2020
@rares-pop
Copy link
Contributor

@rares-pop rares-pop commented Jan 7, 2020

This seems to fix an effect, but not the root cause of the problem.

Also, if it's a general mechanism that 'flattens' the tuples, are there other cases that needs to be fixed?

@srg91
Copy link
Contributor Author

@srg91 srg91 commented Jan 7, 2020

This seems to fix an effect, but not the root cause of the problem.

I think it is, because we have the common func which process loaded cache. We can fix it in another way, but it seems to be more complex, because (if I understand problem correctly) it will require to change the store cache engine (from msgpack to something else).

Also, if it's a general mechanism that 'flattens' the tuples, are there other cases that needs to be fixed?

I looked through grains/core.py and some other modules before and found the only that case.

@arizvisa
Copy link
Contributor

@arizvisa arizvisa commented Jan 8, 2020

Although it might not be the root cause of the problem, when performing an implicit operation that can have different semantics between sets, lists, or tuples, it's much safer to impose the type at the time of use rather than let the type pass through a callstack without knowing what else that type might taint.

Nonetheless, I think it's a maybe good idea to assert the type during development, and impose the type w/ a warning during production to help reduce and track down these sorts of issues. That mis-typing symptom occurs in a few of my other bug reports (#54882 come to mind).

Just my 2cents, and thanks for tracking this down to a caching issue. I literally thought I was crazy when I couldn't repro.

@dwoz
Copy link
Contributor

@dwoz dwoz commented Jan 10, 2020

@srg91 Thanks for the contribution. This change needs to be made against master.

@srg91
Copy link
Contributor Author

@srg91 srg91 commented Jan 10, 2020

@srg91 Thanks for the contribution. This change needs to be made against master.

Thanks for noticing, I just followed the contribution docs. Could you tell, please, the best way to do this? Is it will be enough if I just force replace my branch and change this pull request or I should create the new one?

@srg91 srg91 force-pushed the fix-cached-osrelease_info-grain-type branch from 55413cb to fc9d374 Compare Jan 10, 2020
@srg91 srg91 changed the base branch from 2018.3 to master Jan 10, 2020
@srg91
Copy link
Contributor Author

@srg91 srg91 commented Jan 10, 2020

I've replaced commit with force push and switched destination branch to master, but forgot to update master before, sorry. I've updated branch to master with merge, but maybe I should do this with rebase?

@arizvisa
Copy link
Contributor

@arizvisa arizvisa commented Jan 10, 2020

@srg91, not sure if you'll get a response from the maintainers as I asked questions similar to yours before and didn't get an answer to my questin for like a year. But yeah, once you change the branch compare for your PR to master, the commits are a lot cleaner w/ a rebase.

For now though, just wait for the tests to complete before doing the rebase and I think you should be good.

Before fix the module `loader._load_cached_grains` loaded `osrelease_info` grain as list.
But everythere it expects `tuple`.
Now we force `osrelease_info` type and it works as expected.

Fixes saltstack#54908
@srg91 srg91 force-pushed the fix-cached-osrelease_info-grain-type branch from 9d24dea to 2fc84f4 Compare Jan 10, 2020
@srg91
Copy link
Contributor Author

@srg91 srg91 commented Jan 10, 2020

@arizvisa thanks for your comment. All checks are done instead one - ci/py2/centos7 (https://jenkinsci.saltstack.com/job/pr-centos7-py2/job/PR-55796/3/console). I had no abilities to parse exception and decided to just do rebase :) Maybe restart would help, if no I will try check it later.

@dwoz
Copy link
Contributor

@dwoz dwoz commented Jan 13, 2020

@srg91 @arizvisa Yes, it's been a fairly painful transition to master. The updates to the contributor docs have been committed and will be released with neon.

dwoz
dwoz approved these changes Jan 13, 2020
@dwoz dwoz merged commit 224fb88 into saltstack:master Jan 13, 2020
49 checks passed
@arizvisa
Copy link
Contributor

@arizvisa arizvisa commented Jan 13, 2020

@dwoz, if you guys make tags for prioritizing the PRs your community can likely help with some of the small or submitted ones. Maybe something simple like "PR low-priority", "PR high-priority", "PR medium-priority", etc.

Like I didn't know that you guys were going through the huge process of phasing out the development branch entirely and I can at least help with some of the PRs that me or others have submitted so it's less work on you guys.

But again, I hope I'm not speaking out-of-turn for others, but please let us know if you guys need help with porting one of our PRs. Silence makes us contributors feel hopeless and also whiny.

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

Successfully merging this pull request may close these issues.

None yet

4 participants