bang in outputs does not work correctly

[Elyahou]

Current Behavior

When using bangs in outputs to exclude specific files, it does not work as expected.

For example, if I have this file tree in my project:

dist/
├── bin/
│  ├── file.sh
│  └── file.js
└── test.ts

and I want to add to the outputs cache, all files under dist but excluding the files that are in dist/bin with a sh extension.

The outputs configuration is this case should be:

"outputs": [
        "{projectRoot}/dist/**",
        "!{projectRoot}/dist/bin/*.sh"
      ]

But when running the task, I see that the dist/bin/file.sh is cached in the outputs.

Expected Behavior

The bang allow to exclude some type of files

GitHub Repo

https://github.com/Elyahou/nx-examples/tree/outputs-issue

Steps to Reproduce

1. Clone the fork https://github.com/Elyahou/nx-example and checkout the outputs-issue branch
2. yarn install
3. npx nx repro cart
4. Check the outputs folder under .nx/cache and see that the .sh file is cached...

Nx Report

Node   : 16.15.0
OS     : darwin-arm64
yarn   : 1.22.22

nx                 : 19.1.0-beta.3
@nx/js             : 19.1.0-beta.3
@nx/jest           : 19.1.0-beta.3
@nx/linter         : 19.1.0-beta.3
@nx/eslint         : 19.1.0-beta.3
@nx/workspace      : 19.1.0-beta.3
@nx/angular        : 19.1.0-beta.3
@nx/cypress        : 19.1.0-beta.3
@nx/devkit         : 19.1.0-beta.3
@nx/eslint-plugin  : 19.1.0-beta.3
@nx/react          : 19.1.0-beta.3
@nrwl/tao          : 19.1.0-beta.3
@nx/web            : 19.1.0-beta.3
@nx/webpack        : 19.1.0-beta.3
typescript         : 5.4.3
---------------------------------------
Registered Plugins:
@nx/eslint/plugin
@nx/cypress/plugin
@nx/jest/plugin
---------------------------------------
Community plugins:
@ngrx/component-store : 17.0.1
@ngrx/effects         : 17.0.1
@ngrx/entity          : 17.0.1
@ngrx/operators       : 17.0.0-beta.0
@ngrx/router-store    : 17.0.1
@ngrx/store           : 17.0.1
@ngrx/store-devtools  : 17.0.1

Failure Logs

No response

Package Manager Version

No response

Operating System

• [X] macOS
• [ ] Linux
• [ ] Windows
• [ ] Other (Please specify)

Additional Information

I checked a lot of possibilities and no one work:

"outputs": [
        "{projectRoot}/dist",
        "!{projectRoot}/dist/bin/*.sh"
      ]

"outputs": [
        "{projectRoot}/dist",
        "{projectRoot}/dist/bin/!(*.sh)"
      ]      

"outputs": [
        "{projectRoot}/dist/**/*",
        "!{projectRoot}/dist/bin/*.sh"
      ]      

[netlify[bot]]

✅ Deploy Preview for authentik-storybook canceled.

Name	Link
🔨 Latest commit	614a6ad5d96cc10085ca0296431015fc38ad482e
🔍 Latest deploy log	https://app.netlify.com/sites/authentik-storybook/deploys/676455e729cd9300085c763b

[netlify[bot]]

✅ Deploy Preview for authentik-docs ready!

Name	Link
🔨 Latest commit	614a6ad5d96cc10085ca0296431015fc38ad482e
🔍 Latest deploy log	https://app.netlify.com/sites/authentik-docs/deploys/676455e7a223ed0008642c3c
😎 Deploy Preview	https://deploy-preview-10159--authentik-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[rissson]

Yeah those configuration options have been an issue. I'd rather we remove use_pgbouncer and use_pgpool entirely, and directly expose the django options conn_max_age and disable_server_side_cursors, with pointers in the documentation about what should be set for pgpool and pgbouncer.

Given that those changes would be breaking, we'll not include them in 2024.6

[tomasfarias]

I'd rather we remove use_pgbouncer and use_pgpool entirely, and directly expose the django options conn_max_age and disable_server_side_cursors, with pointers in the documentation about what should be set for pgpool and pgbouncer.

I agree :+1:

Happy to work on a follow-up commit to also expose disable_server_side_cursors and deprecate use_pgbouncer (+ doc updates). Unfortunately, I am not familiar with pgpool to also contribute changes for pgpool settings.

Alternatively, this PR could serve as a stepping stone to provide some quicker relief in a backwards-compatible fashion.

In the meantime, I've been running pgbouncer+Authentik with a patched CONN_MAX_AGE=0, and it's been working great! Thanks for the amazing work in Authentik.

[rissson]

Happy to work on a follow-up commit to also expose disable_server_side_cursors and deprecate use_pgbouncer (+ doc updates). Unfortunately, I am not familiar with pgpool to also contribute changes for pgpool settings.

With pleasure! Let me know if you need any guidance for anything.

You already know about authentik/root/settings.py, but there is also authentik/root/db/base.py which may (?) need some changes.

Docs wise, settings are documentation in website/docs/installation/configuration.mdx. As for release notes, there is a template you can copy from website/docs/releases/_template.md to website/docs/releases/2024/next.md.

We can probably get away with first checking if use_pgpool or use_pgbouncer are set, configure conn_max_age and disable_server_side_cursor, and then override those if they are in the configuration as well. The tricky bit is to make it work for read-replicas in an intuitive way.

As for the documenting the options required for pgpool, it'd be as simple as saying that if pg_pool is used, disable_server_side_cursors must be true.

[tomasfarias]

Just to clarify:

We can probably get away with first checking if use_pgpool or use_pgbouncer are set, configure conn_max_age and disable_server_side_cursor, and then override those if they are in the configuration as well.

So the idea is to keep both use_pgpool and use_pgbouncer? I thought that we were removing them in favor of exposing only conn_max_age and disable_server_side_cursors. Or are we keeping them in a "marked for deprecation" state to ease with the migration?

Keeping them does make it confusing when working with replicas, as use_pgpool and use_pgbouncer are more like flags indicating which settings to use: So, should we check and accept a flag per replica or the global? By removing use_pgpool and use_pgbouncer, we could let folks set replica overrides for conn_max_age and disable_server_side_cursors like any other db setting.

[tomasfarias]

My latest commit goes ahead with deprecating USE_<pooler> settings as it was my original understanding. Granted, I am more than happy to look into bringing them back after my previous comment is clarified, if this was not the intended route. I just happened to have some time now and decided to move forward with my original idea instead of waiting.

The commit also includes documentation changes and next release notes updates as instructed.

[rissson]

My latest commit goes ahead with deprecating USE_ settings as it was my original understanding

Yeah I think that's the best route. I was just saying that in theory we could preserve backwards compatibility.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 89.74359% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.78%. Comparing base (c8711d9) to head (614a6ad). Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
authentik/lib/config.py	80.95%	4 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10159      +/-   ##
==========================================
+ Coverage   91.68%   92.78%   +1.09%     
==========================================
  Files         770      770              
  Lines       38735    38772      +37     
==========================================
+ Hits        35516    35974     +458     
+ Misses       3219     2798     -421     

Flag	Coverage Δ
e2e	48.71% <35.89%> (-0.07%)	:arrow_down:
integration	24.62% <35.89%> (?)
unit	90.38% <89.74%> (+<0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[tomasfarias]

Before we go ahead with this, we'd probably want to check out docs.djangoproject.com/en/dev/releases/5.1/#postgresql-connection-pools as well, just in case changing defaults might make sense

I've taken a look, and it doesn't seem that Django is changing any settings when enabling the internal pool. Moreover the connection pool appears disabled by default.

I think that when/if a AUTHENTIK_POSTGRESQL__POOL setting is ever exposed, then we would have to document how CONN_MAX_AGE and DISABLE_SERVER_SIDE_CURSORS should be set too, as well as the pool specific settings.

[cardboardpig]

Currently seeing the issue fixed by this MR against a Percona Postgres PGBouncer instance; I have set AUTHENTIK_POSTGRESQL__USE_PGBOUNCER to false and AUTHENTIK_POSTGRESQL__USE_PGPOOL to true as a dirty work around to get the pods to stay up while testing but it's not ideal :sweat_smile:

Is there anything I can do to help this along?

[BeryJu]

@tomasfarias Could you look into the changes requested above? Or let us know if you don't have the time/resources to continue with this PR

[tomasfarias]

@BeryJu Hey! Apologies for the long absence. My day job has been quite demanding, and I had some time-off scheduled to recover.

I'll be taking a look at this PR this weekend.

[tomasfarias]

Afaik all conversations should be addressed so this should be good for another review + hopefully merge?

[rissson]

/cherry-pick version-2024.12