Skip to content

add unique name selection and opportunistically generate lower comple…#402

Merged
KCarretto merged 16 commits intospellshift:mainfrom
alexcote1:main
Jan 21, 2024
Merged

add unique name selection and opportunistically generate lower comple…#402
KCarretto merged 16 commits intospellshift:mainfrom
alexcote1:main

Conversation

@alexcote1
Copy link
Copy Markdown
Collaborator

adds unique name selection and opportunistically generate lower complexity beacon names

@hulto hulto added the tavern label Dec 15, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 18, 2023

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (1b8bf51) 66.58% compared to head (02e7391) 71.29%.

Files Patch % Lines
tavern/internal/c2/server.go 50.00% 14 Missing and 3 partials ⚠️
tavern/internal/namegen/namegen.go 80.00% 3 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #402      +/-   ##
==========================================
+ Coverage   66.58%   71.29%   +4.71%     
==========================================
  Files         116      116              
  Lines        8325     8956     +631     
==========================================
+ Hits         5543     6385     +842     
+ Misses       2670     2453     -217     
- Partials      112      118       +6     

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

@alexcote1
Copy link
Copy Markdown
Collaborator Author

do i need to write a test for Beaconnameinstring() and the different complexity levels?

@alexcote1 alexcote1 requested a review from KCarretto December 19, 2023 00:23
@alexcote1 alexcote1 self-assigned this Dec 19, 2023
@alexcote1 alexcote1 marked this pull request as ready for review December 19, 2023 00:23
Copy link
Copy Markdown
Collaborator

@hulto hulto left a comment

Choose a reason for hiding this comment

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

Few nits if you feel inclined.

Copy link
Copy Markdown
Collaborator

@KCarretto KCarretto left a comment

Choose a reason for hiding this comment

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

Primary feedback is to separate API into 3 namegen methods, as that would improve readability quite a bit. Additionally, it appears you're indexing adjectives and nouns when you should be accessing the Halloween variants, which can cause index out of range errors. By swapping to the newAdjective and newNoun helper methods (I demonstrated above), this should hopefully be a bit less confusing to internal package callers. It will also make it significantly easier to add more thematic support in the future.

Otherwise, looks good, thanks for all the work on this!

@cmp5987 cmp5987 mentioned this pull request Jan 15, 2024
@hulto hulto closed this in #435 Jan 15, 2024
@alexcote1
Copy link
Copy Markdown
Collaborator Author

was this fixed in PR #435 ?

@alexcote1 alexcote1 reopened this Jan 18, 2024
KCarretto
KCarretto previously approved these changes Jan 18, 2024
@KCarretto KCarretto merged commit c6c6430 into spellshift:main Jan 21, 2024
KCarretto added a commit that referenced this pull request Feb 1, 2024
 
add unique name selection and opportunistically generate lower comple… (#402)

* add unique name selection and opportunistically generate lower complexity names

* fix tests

* batch search for collisions. and add a complexity identifier

* add test for Beaconnameinstring

* add tests to other random names

* add requested changes.

* make duplicate test use complex names

* impliment one retry per request to generate a name

* rename the complexity levels

* seperate them out into diffrent functions, removing the enum

* fix imports

* add ent to namegen test

---------

Co-authored-by: Hulto <7121375+hulto@users.noreply.github.com>
Co-authored-by: KCarretto <Kcarretto@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants