Orbit spring cleaning#90
Merged
Merged
Conversation
charliemirabile
force-pushed
the
orbit_spring_cleaning
branch
from
ef4d756 to
ac2a0be
Compare
theyoyojo
requested changes
Apr 30, 2024
theyoyojo
left a comment
theyoyojo
left a comment
Contributor
There was a problem hiding this comment.
good stuff. some small comments. tests pass locally
Given that almost all the callers are using get=True, why even have a get parameter at all. Callers can just wrap a call to this function in a call to print to get the behavior of get=False.
Unclear when this would ever be useful (maybe it was added because we used to need to manually edit the synapse db to change passwords?)
A relic of a previous attempt at a dashboard that never was.
unclear when this ever was or will be useful.
It is unclear what this option offers that cannot be achieved by piping the output of roster through grep.
unclear when this would be useful.
unclear when this would be useful when you can just try logging into the website to test the credentials.
The previous approach of printing 'null' or equivalent under error conditions was fallible (consider a user with username null), and not friendly for use in scripts (should exit with nonzero code if error occurred) Instead, for queries that take an action, nothing is printed and the script exits with zero in the happy case, or an error is printed to stderr and the script exits with a nonzero exit code.
This makes the help output a bit nicer
We can just directly show the help output from the arg parser instead of prompting the user to do it themselves with `-h`.
If a command requires more than one argument, list all of the required arguments that are missing instead of only the first one.
the standard library secrets package contains useful functions that can replace the logic we were using for generating tokens. This approach is more secure too because the tokens generated by the previous approach could be brute forced (given a username, and knowing that the token is only good for 3 hours there are finitely many inputs to hash function, the one we were using (SHA256) is designed to be fast to compute which is not what you want for a cryptographically secure hash function i.e. you want slow). A spoofed token could be tested against the website and if a match was found, used to hijack a logged in session.
string already supports `.encode` and bytes already support `.decode`. Introducing new helpers only makes things more confusing.
The decode method is shorter and consistent with the rest of the code. utf-8 is the default option for encoding and decoding, so mentioning it only makes the code longer without subtantially improving the clarity.
This variable is only used one place and just inserted into a different string unconditionally so its inclusion is just more confusing than helpful.
That way, we can use f-strings instead of the old and unpleasent `%` string formatting.
The string is only used one place and putting it in the function means that an f-string can be used which is nicer. Also fix the spacing (wasn't indented), and the closing tags (h1 vs h3 mismatch).
It is only used in one place so just putting it there is nicer.
It is only used in one place and it doesn't even work correctly (missing newlines after each row). We can just write the single table that we want to generate manually and use an f-string too.
`token` and `expiry` were not used anywhere. Accessing
their values only really makes sense if you already know
that session is valid, in which case you can just do
.session.{token,expiry} which is probably clearer.
We aren't using any special versions or exotic packages. Just installing the appropriate global python packages from `apk` is easily adequate to get the dependencies for orbit, and it makes all of the invocations more simple because we do not need to enter a virtual environment anymore since the packages are global.
Since we don't have a venv anymore, we don't need {,dev-}requirements.txt
While we are at it, we can remove the old orbit readme which was an
artifact of it being a separate repo. The instructions are now out of date
and they have been superseded by the main readme.
We also do not need the dockerignore file since the orbit db has not
been stored in the orbit folder in a long time.
There are lots of absolute paths scattered around the orbit codebase. By just setting the workdir to the location of the orbit source folder within the Containerfile most of those paths can be converted into relative paths which makes moving the orbit source code easier.
This makes them consistent with the other `Containerfile`s.
The source code and configuration files can be owned by root because user 100 can still read them. Only the orbit db which needs to be modified by the running container ought to be owned by user 100. This adds an additional layer of security since the code on disk cannot be modified without a privilege escalation to the container root user. Fixes: acf767b ("orbit: switch to non-root user within container")
Instead of copying a bunch of files into the first build stage that are otherwise unused except to copy them from build stage into the final image, just copy directly into the final stage.
Instead of creating a nonstandard orbit directory in the filesystem root, we can put the orbit folder within `/usr/local/share`, a standard location for architecture independent data (e.g. python source code) as specified in the FHS.
charliemirabile
force-pushed
the
orbit_spring_cleaning
branch
from
57b89af to
f1515af
Compare
theyoyojo
reviewed
May 2, 2024
theyoyojo
reviewed
May 2, 2024
theyoyojo
approved these changes
May 2, 2024
theyoyojo
approved these changes
May 2, 2024
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A bunch of changes that occurred to me as I had to wade through most of the code in orbit in order to switch to the ORM. Depends on #89.