-
Notifications
You must be signed in to change notification settings - Fork 8
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
Improve readme #54
Improve readme #54
Conversation
No New Or Fixed Issues Found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great changes! I have added few comments from me that makes it even better.
Answers to the follow-up questions:
- I am fine with moving the contributing instructions to https://contributing.bitwarden.com/.
- The 3.7 is EOL, so from development perspective it make sense to upgrade it, but we can't. Python version have to be locked to 3.7, because Splunk Enterprise works for 3.7 only at this moment and there are at least 2 more years of support for the app on those versions. Their newer release (in beta) brings support to 3.9, but it might take a while. I have not tested newer versions of python. We allow for ranges from 3.7, because it's easier from development perspective and we prepare for the Splunk Enterprise new release that brings 3.9 support. Making it easier from development perspective, so using 3.10 or never is a separate initiative.
README.md
Outdated
|
||
### Local development | ||
1. Clone the Github repository: `git clone https://github.com/bitwarden/splunk.git` | ||
2. Install Python 3.8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's actually 3.7 or newer, but since 3.7 is EOL it might be hard to install. I have tested 3.7, 3.8 and 3.9:
2. Install Python 3.8 | |
2. Install Python 3.8 or 3.9 |
README.md
Outdated
1. Clone the Github repository: `git clone https://github.com/bitwarden/splunk.git` | ||
2. Install Python 3.8 | ||
3. Install [Poetry][poetry] | ||
4. If you're using a Mac, install libmagic: `brew install libmagic` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4. If you're using a Mac, install libmagic: `brew install libmagic` | |
4. If you're using a macOS, install libmagic: `brew install libmagic` |
README.md
Outdated
|
||
Activate shell: `poetry shell` | ||
1. Install Docker | ||
2. If you're using an Apple Silicon Mac, open *Docker* -> *Settings* -> *Features in development* -> enable "Use Rosetta for x86/amd64 emulation on Apple Silicon" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I newer Docker Desktop, it got moved from features in development to General
2. If you're using an Apple Silicon Mac, open *Docker* -> *Settings* -> *Features in development* -> enable "Use Rosetta for x86/amd64 emulation on Apple Silicon" | |
2. If you're using an Apple Silicon Mac, open *Docker Desktop* -> *Settings* -> *General* -> enable "Use Rosetta for x86_64/amd64 emulation on Apple Silicon" |
README.md
Outdated
2. Install Python 3.8 | ||
3. Install [Poetry][poetry] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prerequisites should be first in order, before cloning or navigating to directory
README.md
Outdated
2. Install Python 3.8 | ||
3. Install [Poetry][poetry] | ||
4. If you're using a Mac, install libmagic: `brew install libmagic` | ||
4. Navigate to your local repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct the numbers, we have two 4
s. Also it could be more explicit:
4. Navigate to your local repository | |
4. Navigate to your local repository: `cd splunk` |
README.md
Outdated
4. If you're using a Mac, install libmagic: `brew install libmagic` | ||
4. Navigate to your local repository | ||
5. Activate the poetry shell: `poetry shell` | ||
6. Tell poetry to use Python 3.8: `poetry env use python3.8` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or 3.9, depends on what is installed.
Also it's worth to mention that the python version executable needs to be present in PATH
. Otherwise absolute path to the python executable have to be specified.
README.md
Outdated
- Access logs: | ||
### Deploy the app | ||
|
||
1. Package the app: `./package.sh` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's worth to mention that this script produces a packaged splunk application in this location: output/bitwarden_event_logs.tar.gz
README.md
Outdated
### Deploy the app | ||
|
||
1. Package the app: `./package.sh` | ||
2. Deploy the app to Splunk: `./deploy.sh` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script depends on splunk enterprise dunning in docker.
I think we should move the ### Set up Splunk Enterprise
to be before ### Deploy the app
but after ### Configure your environment
Also the script have the splunk admin's password hardcoded to password
, which we now say you can change to something rlse in the ### Set up Splunk Enterprise
. We should change the deploy.sh
script to have ability to provide the password as argument (defaults to password
).
Also it's worth to mention that the script restarts the splunk and it might become unavailable even after script finishes for few seconds.
poetry.lock
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no need to do it in this PR, since it would require QA testing. Let's leave this to #50 and revert this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was unintentional as I was messing around with my dependencies 😁
Thanks @mzieniukbw , I've continued these changes here: bitwarden/contributing-docs#386 Once that's merged, we can just update this doc to link there. Re: the Python version - can we specify that it should be |
@eliykat i have just tested other python versions and added a suggestion in https://github.com/bitwarden/contributing-docs/pull/386/files |
a2ff4a3
to
6a2550c
Compare
I like specifying it in pyproject.toml, it's an easy thing to miss and it's better to get an explicit error than some random runtime failure. I've done that and regenerated the lockfile as required. Now when I try with 3.12:
I've also updated the README to point to the contributing docs. |
🎟️ Tracking
N/A
📔 Objective
I set up the Splunk app for local development today and hit a few pain points, possibly because I'm not familiar with the Python ecosystem. In particular:
linux/amd64
version of the Splunk image (on a Mac Silicon)I've added these to the instructions, and also restructured them to be more step-by-step.
Two follow-up questions:
pyproject.toml
specifies the python version aspython = ">=3.7.17"
, however these instructions refer to Python 3.8 specifically. I received syntax errors when trying to use 3.12, which is what I already had installed on my system. I suggest we probably need to specify^3.8
- what do you think @WaciX ?⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes