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

feat(auth): integrate ldap #3031

Merged
merged 13 commits into from
Nov 29, 2023
Merged

feat(auth): integrate ldap #3031

merged 13 commits into from
Nov 29, 2023

Conversation

matthewelwell
Copy link
Contributor

Changes

Adds the flagsmith-ldap module as a dependency to the main Flagsmith application. This will be installed as a dependency as part of the flagsmith-private-cloud image build workflow.

How did you test this code?

Tests are run in flagsmith/flagsmith-ldap repository.

# Conflicts:
#	.github/workflows/platform-docker-publish-all-features-image.yml
Copy link

vercel bot commented Nov 24, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 29, 2023 6:05pm
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 29, 2023 6:05pm
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 29, 2023 6:05pm

Copy link
Contributor

github-actions bot commented Nov 24, 2023

Uffizzi Ephemeral Environment deployment-41424

☁️ https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/3031

📄 View Application Logs etc.

What is Uffizzi? Learn more!

@@ -120,7 +120,7 @@ jobs:
context: .
build-args: |
SAML_INSTALLED=1
POETRY_OPTS=--with saml,auth-controller
POETRY_OPTS=--with saml,auth-controller,ldap
Copy link
Member

Choose a reason for hiding this comment

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

Since we're resorting to a private git dependency, this step will fail without adding credentials to the git client.

See one of the solutions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I was assuming that the presence of the github token secret variable would suffice here but perhaps you're right. It's working currently where we manually checkout the packages but I guess that's because we're using a pre-built action. I'll have a play with this...

Copy link
Member

Choose a reason for hiding this comment

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

We are providing a distinct token to the checkout action for the private repos, aren't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right, we are. Well caught, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See successful build here.

@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (aed15c3) 95.74% compared to head (2be63e7) 95.89%.
Report is 9 commits behind head on main.

❗ Current head 2be63e7 differs from pull request most recent head 9efbaf8. Consider uploading reports for the commit 9efbaf8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3031      +/-   ##
==========================================
+ Coverage   95.74%   95.89%   +0.15%     
==========================================
  Files        1045     1039       -6     
  Lines       30889    30688     -201     
==========================================
- Hits        29574    29429     -145     
+ Misses       1315     1259      -56     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -43,6 +43,7 @@ jobs:

- name: Install poetry
run: pipx install poetry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, irrelevant to this PR but it was bothering me.

@matthewelwell matthewelwell added this pull request to the merge queue Nov 29, 2023
Merged via the queue into main with commit 65f78f7 Nov 29, 2023
21 checks passed
@matthewelwell matthewelwell deleted the chore/integrate-ldap branch November 29, 2023 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue related to the REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants