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

Build the package along with the data file for local use #2118

Merged
merged 2 commits into from
May 11, 2024

Conversation

matheusfelipeog
Copy link
Collaborator

hotfix to #2116 (comment)

ref: #2116

@matheusfelipeog
Copy link
Collaborator Author

Don't merge yet, I'm double-checking this.

@ppfeister
Copy link
Member

ppfeister commented May 11, 2024

This looks like a good fix and should be applied, but I have some thoughts related to this...

What's the point of --local for packaged builds?

--local is much needed for testing and such when dealing with the source, but is there any real use for it when packaged, where the user isn't likely to change it anyways?

(merge should happen either way if functional --- just related)

@matheusfelipeog
Copy link
Collaborator Author

Don't merge yet, I'm double-checking this.

Ready, everything seems fine now. The dynamic search mechanism in the package build wasn't working well for some reason, so it's better to keep the explicit definition.

Copy link
Member

@ppfeister ppfeister left a comment

Choose a reason for hiding this comment

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

Fix works on my end.

@matheusfelipeog
Copy link
Collaborator Author

What's the point of --local for packaged builds?

--local is much needed for testing and such when dealing with the source, but is there any real use for it when packaged, where the user isn't likely to change it anyways?

Good point. I also thought about that when I was analyzing it.

I don't see a use for the average user. It's just a feature for testing.

Maybe we should mention this in the documentation?

@ppfeister
Copy link
Member

Good point. I also thought about that when I was analyzing it.

I don't see a use for the average user. It's just a feature for testing.

Maybe we should mention this in the documentation?

I was planning to add some checks soon where if sherlock is ran on ${platform} certain options would change or be excluded. Just due to how things are packaged and such. I could create an Issue to dynamically remove the option when someone gets to it, rather than adding to docu

(issue to track)

@matheusfelipeog
Copy link
Collaborator Author

I'm not sure if this is necessary for the --local case. It doesn't seem to have any strange behavior when keeping it.

Since data.json will be part of the build, users with older versions installed will have access to old data using --local. Maybe this could be leveraged somehow? Comparisons with the version of remote data? Just thoughts...

@matheusfelipeog matheusfelipeog merged commit f8e3bd7 into master May 11, 2024
3 checks passed
@matheusfelipeog matheusfelipeog deleted the hotfix/local-flag-and-data-file branch May 11, 2024 20:53
@ppfeister
Copy link
Member

hm. what if we set --local to use ~/.config/sherlock/data.json either instead of packaged data.json or before packaged data.json

that would allow packaged users to still take advantage of the option without having to detect what version they were on

only issue would be adapting this for windows users, since I am not one and don't know those conventions. appdata, maybe?

@sdushantha
Copy link
Member

Thank you @matheusfelipeog for noticing and fixing this!


What's the point of --local for packaged builds?

--local is much needed for testing and such when dealing with the source, but is there any real use for it when packaged, where the user isn't likely to change it anyways?

@ppfeister, --local can be used by users who have a custom data.json file with sites that they are using during their OSINT investigation, but dont want to be added to Sherlock.

@ppfeister
Copy link
Member

ppfeister commented May 12, 2024

@sdushantha

@ppfeister, --local can be used by users who have a custom data.json file with sites that they are using during their OSINT investigation, but dont want to be added to Sherlock.

When packaged, however, that source directory is pretty buried. Users who want to load their own manifest may be better suited by using --json, which does the exact same thing but would allow them to point Sherlock anywhere.

When packaged as an rpm, for instance, the data.json file resides at /usr/lib/python3.12/site-packages/resources/data.json. Not the most obvious place. Very doable, just not convenient or obvious.


A better solution in my mind would be to scrap the --local flag, and make data.json the default argument for --json when no argument is provided. This would reduce the number of flags needed without losing any functionality, simplifying sherlock.

Also, when packaged, I don't see this inclusion being very valuable in such an odd spot. In my mind, it would make more sense to drop data.json when packaged, and have the default location for --json become (only when packaged) ~/.config/sherlock/data.json for in the event a user specified one is present.

@sdushantha
Copy link
Member

@ppfeister

When packaged, however, that source directory is pretty buried.

This is very true. I for some reason through that --local was able to take a path to a .json file, but that is not the case. So i agree with your suggestion on scrapping --local and using --json instead. What are your thoughts on this @matheusfelipeog?

@matheusfelipeog
Copy link
Collaborator Author

What are your thoughts on this @matheusfelipeog?

@sdushantha Although both (--local and --json) have similar behaviors, to me their purposes are slightly different:

  • The --local flag is essentially a feature for testing when there's an addition to the local data.json file.

  • With the --json flag, the user can load customizable data that they don't want to directly add to the Sherlock upstream for some reason.

--local doesn't have its initial usefulness when packaged, true, but I don't think that's a reason to discard it. I would rather suggest informing in the CLI documentation that it's essentially a feature for developers and contributors of sites for tracking. And anyway, everything will continue to work fine.

@ppfeister
Copy link
Member

--json currently requires an argument (some sort of uri to a json file). If we were to simply set the default value to resources/data.json, that would negate --local entirely, even for devs, no?

When testing changes to their local json, they would just run sherlock --json (with no args) rather than sherlock --local, simplifying the codebase a bit in the process

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

3 participants