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

SYN-7653: Migration for invalid it:sec:cpe nodes #3815

Open
wants to merge 54 commits into
base: master
Choose a base branch
from

Conversation

MichaelSquires
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.29%. Comparing base (8120522) to head (3e8b31e).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3815      +/-   ##
==========================================
- Coverage   96.36%   96.29%   -0.07%     
==========================================
  Files         242      243       +1     
  Lines       53771    53862      +91     
==========================================
+ Hits        51816    51869      +53     
- Misses       1955     1993      +38     
Flag Coverage Δ
linux 96.29% <100.00%> (+<0.01%) ⬆️
linux_replay ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -121,7 +121,8 @@ commands:
- run:
name: checkout regression repo
command: |
git clone https://github.com/vertexproject/synapse-regression ~/git/synapse-regression
# FIXME: Remove branch before merging
Copy link
Contributor Author

@MichaelSquires MichaelSquires Jul 20, 2024

Choose a reason for hiding this comment

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

🎶 🎶 🎶
Don't you forget about me
Don't, don't, don't, don't
Don't you forget about me
🎶 🎶 🎶

@MichaelSquires
Copy link
Contributor Author

Regression repo here: vertexproject/synapse-regression#23

@MichaelSquires MichaelSquires marked this pull request as ready for review July 22, 2024 18:05
Copy link
Contributor

@invisig0th invisig0th left a comment

Choose a reason for hiding this comment

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

couple nits but this is looking really good

synapse/lib/modelrev.py Outdated Show resolved Hide resolved
synapse/assets/storm/migrations/model-0.2.27.storm Outdated Show resolved Hide resolved
synapse/assets/storm/migrations/model-0.2.27.storm Outdated Show resolved Hide resolved
.coveragerc Outdated Show resolved Hide resolved
MichaelSquires and others added 2 commits July 29, 2024 12:09
Co-authored-by: mikemoritz <57907149+mikemoritz@users.noreply.github.com>
synapse/lib/stormlib/model.py Outdated Show resolved Hide resolved
synapse/lib/stormlib/model.py Outdated Show resolved Hide resolved
synapse/lib/stormtypes.py Outdated Show resolved Hide resolved
…ertexproject/synapse into blackout/SYN-7653/it-sec-cpe-migration
synapse/lib/stormtypes.py Outdated Show resolved Hide resolved
synapse/tests/test_lib_stormtypes.py Outdated Show resolved Hide resolved
@@ -603,6 +604,7 @@ workflows:
branches:
only:
- master
- blackout/SYN-7653/it-sec-cpe-migration
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't forget about me either.

@MichaelSquires MichaelSquires force-pushed the blackout/SYN-7653/it-sec-cpe-migration branch 2 times, most recently from fe186ad to 8daefb7 Compare August 5, 2024 19:06
vEpiphyte added a commit that referenced this pull request Aug 6, 2024
Originally from #3815 - credit to @MichaelSquires

---------

Co-authored-by: blackout <blackout@vertex.link>
migrate invalid ``it:sec:cpe`` nodes.

Migration of these nodes will only be successful if one of the CPE2.3 (primary
property) or the CPE2.2 (``:v2_2``) strings are valid CPE. If both CPE
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
property) or the CPE2.2 (``:v2_2``) strings are valid CPE. If both CPE
property) or the CPE2.2 (``:v2_2``) strings are valid CPEs. If both CPE

{'name': 'all', 'type': 'boolean', 'default': False,
'desc': 'Remove all instances of item from the list.'},
),
'returns': {'type': 'null'}}},
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like it returns a bool

if start in text and block is not None:
continue

if start in text:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could merge these with the previous blocks to avoid checking for start/stop in the line twice

Comment on lines +262 to +267
while $list.has($oldvalu) {
$list.rem($oldvalu)
}

if (not $list.has($newvalu)) {
$list.append($newvalu)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be getting too far into edge cases here but this will change the order of unsorted array props and could remove multiple but only re-add one instance in non-uniq array props

($verb, $dst) = $edge
$edges.append($edge)

[ -($verb)> { iden $dst } ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[ -($verb)> { iden $dst } ]
[ -($verb)> { yield $dst } ]


spin |

iden $iden |
Copy link
Contributor

Choose a reason for hiding this comment

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

the yield keyword is more efficient than the iden command

Suggested change
iden $iden |
yield $iden |

($verb, $src) = $edge
$edges.append($edge)

[ <($verb)- { iden $src } ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[ <($verb)- { iden $src } ]
[ <($verb)- { yield $src } ]


spin |

iden $oldcpe.iden() |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
iden $oldcpe.iden() |
yield $oldcpe.iden() |


$removeNode($oldcpe, $view.iden, $view.layers.0.iden)

iden $oldcpe.iden() |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
iden $oldcpe.iden() |
yield $oldcpe.iden() |

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

5 participants