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

refactor: quickstart.yml #2086

Closed
wants to merge 2 commits into from
Closed

refactor: quickstart.yml #2086

wants to merge 2 commits into from

Conversation

rreo
Copy link

@rreo rreo commented Dec 28, 2021

This command would fail without 'serve' in front. Fixing it however does not affect anything, therefore, removing it for better clarity.

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

This command would fail without 'serve' in front. Fixing it however does not affect anything, therefore, removing it for better clarity.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@rreo rreo changed the title Update quickstart.yml refactor: quickstart.yml Dec 28, 2021
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you! However, I think that the config is needed for the migration. Unless this causes a bug I think we should keep it :)

Add 'serve' to command for successful command execution in container.
@@ -13,7 +13,7 @@ services:
- type: bind
source: ./contrib/quickstart/kratos/email-password
target: /etc/config/kratos
command: -c /etc/config/kratos/kratos.yml migrate sql -e --yes
command: serve -c /etc/config/kratos/kratos.yml migrate sql -e --yes
Copy link
Member

Choose a reason for hiding this comment

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

The command written there is correct. serve ... migrate is not a valid command, and it will fail. What problem are you encountering or trying to solve?

Copy link
Author

Choose a reason for hiding this comment

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

Apologies in advance, I'm a new user and I was following the guide at https://www.ory.sh/kratos/docs/guides/docker/ to spin up a docker instance. My objective was merely to successfully spin up an instance.

Using that link as reference I tried to run this code in powershell

docker run `
-it `
-e DSN="memory"  `
--mount type=bind,source="$(pwd)",target=/etc/config/kratos `
oryd/kratos:latest-sqlite `
--config /etc/config/kratos/kratos.yml

The output was

Error: unknown flag: --config
Usage:
  kratos [command]

Available Commands:
  completion  generate the autocompletion script for the specified shell
  courier     Commands related to the Ory Kratos message courier
  hashers     This command contains helpers around hashing
  help        Help about any command
  identities  Tools to interact with remote identities
  jsonnet     Helpers for linting and formatting JSONNet code
  migrate     Various migration helpers
  remote      Helpers and management for remote Ory Kratos instances
  serve       Run the Ory Kratos server
  version     Show the build version, build time, and git hash

Flags:
  -h, --help   help for kratos

Use "kratos [command] --help" for more information about a command.

unknown flag: --config

Then I referred quickstart.yml trying to find a working code. Noticed line 41 had a 'serve' and I successfully spin up an instance as well

docker run `
-it `
-e DSN="memory"  `
--mount type=bind,source="$(pwd)",target=/etc/config/kratos `
oryd/kratos:v0.8.0-alpha.3-sqlite `
serve --config /etc/config/kratos/kratos.yml

And then I wrongly assumed line 16 was wrong based on my experience and made this pull request, my apologies. I was confused by this inconsistency of commands between two supposedly similar docker images. I ran the serve ... migrate code and indeed it gave an 'unknown shorthand 'e' in -e' error.

@rreo
Copy link
Author

rreo commented Dec 29, 2021

I checked the image history of oryd/kratos:latest-sqlite with docker desktop and verified it had CMD ['serve'] during image build so I'm still confused why I was required to manually put the 'serve' command during docker run.

Regardless, you can close this and maybe update that docker doc guide to have others avoid my experience. Thanks! :)

@aeneasr
Copy link
Member

aeneasr commented Dec 30, 2021

Oh wow, the page at https://www.ory.sh/kratos/docs/guides/docker/ is quite outdated! In general it's not a good idea to run latest tags of something. I will fix that in a PR :)

aeneasr added a commit that referenced this pull request Dec 30, 2021
@aeneasr aeneasr mentioned this pull request Dec 30, 2021
7 tasks
aeneasr added a commit that referenced this pull request Dec 30, 2021
peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants