Skip to content

feat: Introduce getSlug endpoint #74

Merged
aeneasr merged 25 commits intoory:masterfrom
Demonsthere:feat/use-project-slug-endpoint
May 14, 2021
Merged

feat: Introduce getSlug endpoint #74
aeneasr merged 25 commits intoory:masterfrom
Demonsthere:feat/use-project-slug-endpoint

Conversation

@Demonsthere
Copy link
Copy Markdown
Contributor

@Demonsthere Demonsthere commented May 5, 2021

Related issue

Proposed changes

  • Switch from using --project flag to retrieving the project slug from dedicated endpoint

Checklist

  • I have read the contributing guidelines
    and signed the CLA.
  • 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
    security@ory.sh) 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 necessary documentation within the code base (if
    appropriate).

Further comments

Copy link
Copy Markdown
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.

That's pretty much what I imagined! :)

Comment thread cmd/cloud/remote/client.go Outdated
Comment thread cmd/cloud/remote/client.go Outdated
@Demonsthere Demonsthere marked this pull request as ready for review May 5, 2021 19:31
@Demonsthere Demonsthere requested a review from aeneasr May 6, 2021 09:23
Comment thread cmd/cloud/remote/client.go Outdated

go func() {
stdout, stderr, err := newCommand(t, ctx).Exec(os.Stdin, "proxy", upstream.URL, "--"+proxy.NoCertInstallFlag, "--"+remote.FlagProject, "demo", "--"+proxy.PortFlag, fmt.Sprintf("%d", port), "--"+proxy.ProtectPathsFlag, "/private/1", "--"+proxy.ProtectPathsFlag, "/private/2")
stdout, stderr, err := newCommand(t, ctx).Exec(os.Stdin, "proxy", upstream.URL, "--"+proxy.NoCertInstallFlag, "--"+proxy.PortFlag, fmt.Sprintf("%d", port), "--"+proxy.ProtectPathsFlag, "/private/1", "--"+proxy.ProtectPathsFlag, "/private/2")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am very astonished that the tests are passing. Something can't be right!

@Demonsthere Demonsthere marked this pull request as draft May 6, 2021 15:23
Copy link
Copy Markdown
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.

This looks good, just needs some tests now!

@Demonsthere Demonsthere marked this pull request as ready for review May 12, 2021 18:27
@Demonsthere Demonsthere requested a review from aeneasr May 12, 2021 18:27
Comment thread .circleci/config.yml Outdated
working_directory: /go/src/github.com/ory/cli
steps:
- checkout
- run: sudo bash -c "export PATH=$PATH:/home/circleci/.local/bin:/home/circleci/bin:/go/bin:/usr/local/go/bin:/usr/local/sbi && make test"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Demonsthere I reverted this because it did not look relevant for the CI to work.

Comment thread cmd/cloud/identities/list_test.go Outdated
"context"
"encoding/json"
"fmt"
"github.com/julienschmidt/httprouter"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Demonsthere please run make format before requesting review

}

func TestIdentityListNoToken(t *testing.T) {
// See https://github.com/ory-corp/cloud/issues/865
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Demonsthere I disabled the tests for now - we can reenable them once this is merged into cloud

Comment on lines +66 to +67
c := retryablehttp.NewClient()
c.Logger = nil
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Demonsthere the CLI printed several debug outputs:

2021/05/14 12:07:46 [DEBUG] GET https://api.console.ory.sh/backoffice/token/slug
2021/05/14 12:07:47 [DEBUG] GET https://playground.projects.oryapis.com/api/kratos/admin/identities

This really confuses people. We often get issues where people ask about a warning or a log entry because they fear something does not work. They expect to be no output expect the one they ... expect. So when writing these CLI commands we need to ensure that no random debug statements get printed. Also, here, my log level was not set to DEBUG which should have been the case if we actually want to see some debugging logs.

func RegisterClientFlags(flags *pflag.FlagSet) {
flags.String(FlagAPIEndpoint, "https://oryapis.com", "Use a different endpoint.")
flags.String(FlagConsoleAPI, "https://console.ory.sh", "Use a different URL.")
flags.String(FlagConsoleAPI, "https://api.console.ory.sh", "Use a different URL.")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Demonsthere the default arguments here was incorrect

Comment thread go.mod
github.com/ory/jsonschema/v3 v3.0.3
github.com/ory/kratos v0.6.0-alpha.2
github.com/ory/kratos-client-go v0.5.4-alpha.1.0.20210308170950-06c2c1c071a8
github.com/ory/kratos-client-go v0.6.1-alpha.1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Demonsthere cloud runs 0.6.1 so I've updated the SDK dependency

@aeneasr aeneasr merged commit f00265a into ory:master May 14, 2021
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.

2 participants