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.

Sign up for GitHub

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

Jump to bottom

feat: Add init api tokens option #1181

Merged
merged 8 commits into from
Jan 5, 2022
Merged

feat: Add init api tokens option #1181

merged 8 commits into from
Jan 5, 2022

Conversation

mjuraj
Copy link
Contributor

@mjuraj mjuraj commented Dec 13, 2021 •
edited by ivarconr

Issue: #1153

  • Tested manually with token value defined in server-dev
  • Tested manually by using INIT_ADMIN_API_TOKENS env variable
  • Added unit tests

fixes #1153

@vercel
Copy link

vercel bot commented Dec 13, 2021 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/unleash-team/unleash-docs/DBwWdCsapVsCZzNxudTkbgaMMyTT
✅ Preview: https://unleash-docs-git-fork-mjuraj-feature-init-a-343362-unleash-team.vercel.app

@mjuraj mjuraj mentioned this pull request Dec 13, 2021
Closed
ivarconr
ivarconr reviewed Dec 13, 2021
View reviewed changes
src/lib/services/api-token-service.ts Outdated
project: tokenParts[0],
environment: tokenParts[1],
secret: `${tokenParts[0]}:${tokenParts[1]}.${tokenParts[2]}`,
type: ApiTokenType.ADMIN,
Copy link
Member

@ivarconr ivarconr Dec 13, 2021 •
edited

Choose a reason for hiding this comment

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

Are we sure it only makes sense to only support Admin tokens as part of init? I am leaning towards yes, because you can use these to create client SDK tokens with the Admin API token.

What do you think @chriswk?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems reasonable, as you say, that will allow you to use the created admin token to create necessary client tokens.

@ivarconr
Copy link
Member

I will try to create an automated test for this before it is merged.

@ivarconr ivarconr self-requested a review January 4, 2022 15:08
@ivarconr
Copy link
Member

ivarconr commented Jan 4, 2022

I have refactored the code a bit:

  • do environment variable => ApiToken conversion while loading the config.
  • only accept API tokens of type admin via env variable, while accepting any token types as a programmatic option.

In addition I have added tests 🚀

@ivarconr
Copy link
Member

ivarconr commented Jan 4, 2022

Feature/init api tokens

@ivarconr ivarconr changed the title Feature/init api tokens feat: Add init api tokens option Jan 5, 2022
sighphyre
sighphyre approved these changes Jan 5, 2022
View reviewed changes
Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

Looks great! Some very small, very optional language things in the docs section at the end

website/docs/deploy/configuring-unleash.md Outdated Show resolved Hide resolved
website/docs/deploy/configuring-unleash.md Outdated Show resolved Hide resolved
@ivarconr @sighphyre
Update website/docs/deploy/configuring-unleash.md
f7b1119 
Co-authored-by: sighphyre <liquidwicked64@gmail.com>
@ivarconr @sighphyre
Update website/docs/deploy/configuring-unleash.md
df087ca 
Co-authored-by: sighphyre <liquidwicked64@gmail.com>
@ivarconr ivarconr merged commit e757c00 into Unleash:master Jan 5, 2022
thomasheartman
thomasheartman requested changes Jan 5, 2022
View reviewed changes
Copy link
Contributor

@thomasheartman thomasheartman left a comment

Choose a reason for hiding this comment

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

Nice! I have a couple minor change suggestions. I also have a few questions that I would love to have clarified if you've got the time - they're inline in that case 😇

website/docs/deploy/configuring-unleash.md Show resolved Hide resolved
website/docs/deploy/configuring-unleash.md Outdated Show resolved Hide resolved
website/docs/deploy/configuring-unleash.md Show resolved Hide resolved
website/docs/deploy/configuring-unleash.md Show resolved Hide resolved
Tymek pushed a commit that referenced this pull request Aug 26, 2022
@olav
refactor: fix small issues around custom strategies (#1181)
710ebe0 
* refactor: validate strategy name on blur

* refactor: remove strategy parameter type text in favor of docs

* refactor: improve pie chart rendering

* refactor: show icons for all feature strategies

* refactor: fix list parameter add button style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivarconr ivarconr ivarconr approved these changes

@thomasheartman thomasheartman thomasheartman requested changes

@sighphyre sighphyre sighphyre approved these changes

@chriswk chriswk Awaiting requested review from chriswk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for default API token in docker
5 participants
@mjuraj @ivarconr @chriswk @thomasheartman @sighphyre