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

Use packaged msgpack #55354

Merged
merged 5 commits into from
Dec 18, 2019
Merged

Use packaged msgpack #55354

merged 5 commits into from
Dec 18, 2019

Conversation

Akm0d
Copy link
Contributor

@Akm0d Akm0d commented Nov 18, 2019

What does this PR do?

Separates msgpack changes from #54878
Adds a significant amount of testing to new msgpack module

@Akm0d Akm0d requested a review from a team as a code owner November 18, 2019 21:59
@ghost ghost requested a review from DmitryKuzmenko November 18, 2019 21:59
@dwoz
Copy link
Contributor

dwoz commented Nov 19, 2019

re-run full all

@dwoz dwoz requested a review from waynew November 19, 2019 17:16
salt/log/handlers/fluent_mod.py Outdated Show resolved Hide resolved
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

🚀

@Akm0d Akm0d force-pushed the msgpack branch 2 times, most recently from b24705d to dd47818 Compare December 11, 2019 19:48
@Akm0d Akm0d requested a review from dwoz December 12, 2019 21:31
@codecov
Copy link

codecov bot commented Dec 15, 2019

Codecov Report

Merging #55354 into master will decrease coverage by 21.62%.
The diff coverage is 40.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #55354       +/-   ##
===========================================
- Coverage   40.05%   18.43%   -21.61%     
===========================================
  Files        1484      816      -668     
  Lines      261512   174097    -87415     
  Branches    56214    37405    -18809     
===========================================
- Hits       104717    32077    -72640     
+ Misses     144600   139454     -5146     
+ Partials    12195     2566     -9629
Flag Coverage Δ
#archlts 17.72% <39.67%> (-0.34%) ⬇️
#centos7 23% <43.4%> (-1.58%) ⬇️
#cloud ?
#debian8 ?
#debian9 ?
#m2crypto ?
#proxy 23.04% <43.4%> (-0.66%) ⬇️
#py2 ?
#py3 18.43% <40.66%> (-1%) ⬇️
#pycryptodomex ?
#runtests 18.43% <40.66%> (-21.61%) ⬇️
#ubuntu1604 22.99% <43.4%> (-15.98%) ⬇️
#ubuntu1804 ?
#zeromq 18.43% <40.66%> (-20.67%) ⬇️
Impacted Files Coverage Δ
salt/states/pkg.py 6.23% <0%> (-4.98%) ⬇️
salt/modules/saltcheck.py 13.83% <100%> (-28.45%) ⬇️
salt/modules/winrepo.py 38.99% <100%> (+0.28%) ⬆️
salt/utils/http.py 11.31% <14.29%> (-25.65%) ⬇️
salt/utils/cache.py 31.74% <20%> (-40.77%) ⬇️
salt/modules/state.py 19.98% <20%> (-52.86%) ⬇️
salt/serializers/msgpack.py 27.28% <28.58%> (-7.05%) ⬇️
salt/payload.py 29% <33.34%> (-28.33%) ⬇️
salt/returners/local_cache.py 20.13% <33.34%> (-25.55%) ⬇️
salt/transport/ipc.py 45.51% <40%> (-24.63%) ⬇️
... and 1188 more

@Akm0d
Copy link
Contributor Author

Akm0d commented Dec 15, 2019

Cleaned up the history now that it's all green 👍. @dwoz I tried to move as much msgpack logic as possible from salt/payload.py to salt/utils/msgpack.py. If I move much more then the msgpack module starts behaving too differently from the base msgpack module

Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Does this replace all of the import msgpack?

It would be full of winning if we could add a lint rule to stop any import msgpack - though I'm not sure if those rules are in this repo.

@Akm0d
Copy link
Contributor Author

Akm0d commented Dec 16, 2019

Does this replace all of the import msgpack?

It would be full of winning if we could add a lint rule to stop any import msgpack - though I'm not sure if those rules are in this repo.

In most cases, yes, It replaces most import msgpack. The exceptions are a few linters, loggers, and tests where it would cause a circular dependency

smarsching and others added 5 commits December 17, 2019 14:49
This commit introduces salt.utils.msgpack modifies all places in the
code that either use json or msgpack to use salt.utils.json or
salt.utils.msgpack respectively.

While this change itself does not have any effect, it is important to
allow for centrally dealing with objects that cannot be directly
serialied via json or msgpack.
When accessing msgpack.Unpacker, we cannot use salt.utils.msgpack, but
we do not have to either.
@Akm0d
Copy link
Contributor Author

Akm0d commented Dec 18, 2019

re-run full debian8

@Akm0d Akm0d self-assigned this Dec 18, 2019
@Akm0d Akm0d added the Core relates to code central or existential to Salt label Dec 18, 2019
@Akm0d Akm0d added this to the Approved milestone Dec 18, 2019
@Akm0d
Copy link
Contributor Author

Akm0d commented Dec 18, 2019

re-run full macosxmojave

@dwoz dwoz merged commit fe5c19f into saltstack:master Dec 18, 2019
@Akm0d Akm0d deleted the msgpack branch December 19, 2019 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core relates to code central or existential to Salt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants