website/docs: Add steps for fixing xml python errors, clean up#16223
website/docs: Add steps for fixing xml python errors, clean up#16223tanberry merged 2 commits intogoauthentik:mainfrom
Conversation
✅ Deploy Preview for authentik-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for authentik-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for authentik-integrations ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16223 +/- ##
==========================================
- Coverage 92.78% 92.73% -0.06%
==========================================
Files 838 838
Lines 45290 45290
==========================================
- Hits 42023 41999 -24
- Misses 3267 3291 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| ## End-to-End (E2E) Setup | ||
|
|
||
| To run E2E tests, navigate to the `/tests/e2e` directory in your local copy of the authentik git repo, and start the services by running `docker compose up -d`. | ||
| Start the e2e test services with the following command: |
There was a problem hiding this comment.
@ConzorKingKong I'm a big fan of telling people where they need to be when they run a command, so at first I was wondering why you removed that, but I see it is part of the command itself, so they can run it from wherever, right?
There was a problem hiding this comment.
Technically the command has to be ran from the root of the folder that the app is in. But this fits better with how we handle the other docker compose script in the initial steps and all the make commands. Basically every other command is set to run in the root of the project and this change makes this fit that pattern
| Don't forget to add `krb5` to your path or else you may run into problems with Kerberos functionality: | ||
|
|
||
| ```shell | ||
| echo 'export PATH="$(brew --prefix krb5)/bin:$PATH"' >> ~/.bashrc |
There was a problem hiding this comment.
default mac shell is zsh not bash
There was a problem hiding this comment.
Thank you, I remember having to install zsh on (i think linux) many moons ago, never clicked that its default on mac
| Install the required dependencies on macOS using Homebrew: | ||
|
|
||
| ```shell | ||
| xcode-select --install |
There was a problem hiding this comment.
i dont think we need this for docs only?
There was a problem hiding this comment.
This installs make. we could say brew install it, but ive never installed make that way
There was a problem hiding this comment.
Creates an unneeded dependency on xcode, brew is easier
There was a problem hiding this comment.
nvm doesn't look like it, but it's an extra program one needs to install
There was a problem hiding this comment.
I mean it's pretty benign overall. If someone runs it and they already have x-code installed, it skips it. and if they dont have it installed, they probably should. but its a long install.
currently we list javascript and make as pre-reqs. however, we add installing node to our scripts on the full dev setup. I added make just because it made the installs bulletproof, but adding pre-reqs to the install scripts kind of breaks the pattern that we usually hold which is "prereqs are pre requirements, install them on your own beforehand".
So I think if I'm removing xcode, I should remove nodejs from the install scripts. or remove them both from the pre-reqs.
To be honest I'm not very strongly held to any option. we can
- remove nodejs and make from the prereqs and keep the install scripts
- keep nodejs and make in the prereqs and remove them from both docs pages install scripts (meaning docs page has no install scripts)
- keep them both in the prereqs and the install to both guarantee the user has the proper things installed and the user has context of what is being installed by it being in the prereqs (currently implemented in the PR)
There was a problem hiding this comment.
For now, I removed xcode install from install steps and left everything else as is. The prereqs wording is a minor but notable thing. I'm willing to come to an answer on it or just leave it for another PR
There was a problem hiding this comment.
Thank you. Having fought with Xcode and all those gems and jewels, yeah, I am not a fan.
There was a problem hiding this comment.
Node is a requirement for Docs too, I am pretty sure. I vote for #3 of those choices above, but I would like @GirlBossRush to take a look too please.
|
|
||
| ## 2. Instructions | ||
|
|
||
| ### Clone the repo |
There was a problem hiding this comment.
do we really need headers? it's just codeblocked commands with no text and takes up unneeded space imo
There was a problem hiding this comment.
changed to sentences
| make docs | ||
| ``` | ||
|
|
||
| Now you can push up your changes and send us a pull request. |
There was a problem hiding this comment.
I feel like this lacks description compared to the rest. I'd suggest looking at our other guides for reference
| Since macOS includes a default installation of `libxml2`, Python may fail to build `xmlsec` and `lxml` correctly when using the `libxml2` version provided by `libxmlsec1`, unless you update your build flags. | ||
|
|
||
| ```shell | ||
| echo 'export LDFLAGS="-L$(brew --prefix libxml2)/lib $LDFLAGS"' >> ~/.zshrc |
There was a problem hiding this comment.
also, do we really want to define LDFLAGS on our entire system just for this singular xml thing? Seems like a bad idea
There was a problem hiding this comment.
ldflags and the 2 below as well*
There was a problem hiding this comment.
I can try and add them manually to the makefile. its a bad idea for the user to not have them set imo. they'll just get stuck using the old version. but we can ignore giving the user instructions and try to update the makefile directly. May need someone like jens to check this pr out now though, I'm not a make expert or anything, could be unintended side effects, but it should be fine
BeryJu
left a comment
There was a problem hiding this comment.
👍 also good idea with adding it to the makefile
| ```shell | ||
| sudo apt-get install -y \ | ||
| build-essential \ | ||
| nodejs |
There was a problem hiding this comment.
Many distros ship with old versions of node. Debian trixie packages node 20 (https://packages.debian.org/trixie/nodejs) and ubuntu lts packages with node 18 (https://packages.ubuntu.com/noble/nodejs). Anyways, NVM is the recommended installation method... See: https://nodejs.org/en/download
There was a problem hiding this comment.
True, @ConzorKingKong since we require v.24, we should make it clear that whatever tool installs Node, it needs to be v 24.
|
Hiya @ConzorKingKong I think the only thing that needs to be done still for this PR is to modify the section that installs node without enforcing v 24. Let me know when that's done and let's get this merged. Thanks! |
|
@tanberry alright, I can do that. The only good way to really do this on linux is to have people install a version manager and install through that. Let me try to make a combination that is straightforward and clean for this |
|
I asked them to download the latest version from the nodejs website and linked to it |
|
|
||
| ## Prerequisites | ||
|
|
||
| - [Node.js](https://nodejs.org/en) (24 or later) |
There was a problem hiding this comment.
| - [Node.js](https://nodejs.org/en) (24 or later) | |
| - [Node.js v24](https://nodejs.org/en) |
First of all 24 is the latest, but also with all the javascript world changes and compatibility/reqs it's best to pin to a specific major version
There was a problem hiding this comment.
The version "24 or later" was written like that because it was written that way in the full development environment and frontend development environment pages. If we change this, then we have to update those locations and all of our other prereqs to change their version location. For now, I'd like to focus on getting this PR in for the Makefile changes and then circle back to this later
The frontend development environment page needs some refreshing to match these pages, so we could make a PR later for that page and spruce this up in that PR
There was a problem hiding this comment.
Yeah I can come back to it after this is all merged in it's nbd
| import TabItem from "@theme/TabItem"; | ||
| import Tabs from "@theme/Tabs"; | ||
|
|
||
| If you want to only make changes to the documentation, you only need Node.js. |
There was a problem hiding this comment.
Does one also need Make, as the current docs say? @ConzorKingKong
tanberry
left a comment
There was a problem hiding this comment.
Let's merge it @ConzorKingKong !
Details
There is a bug with the SAML tests i've been able to reproduce across 3 macs. I have found the solution and fixed it across 3 macs as well. I added the steps to fix this into the dev documentation, as if this isn't fixed, when someone runs
make all, their tests will never pass.I did some cleanup as well in the docs while I was in there.
I made a change to the order that
make allfires for sanity, tests at the end feels a bit better because failing ongenorwebafterwards can feel frustratingChecklist
ak test authentik/)make lint-fix)If an API change has been made
make gen-build)If changes to the frontend have been made
make web)If applicable
make docs)