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: add data import/export commands #156

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yann-soubeyrand
Copy link

This simplifies the provisioning of local development environments. One can setup its organization, project, users, etc, and export instance data to a JSON file. Then, when creating a new development environment, the JSON file can be imported to the newly created instance.

Copy link

codecov bot commented Jun 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.97%. Comparing base (f3faabd) to head (5a7be1f).
Report is 54 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #156      +/-   ##
==========================================
+ Coverage   91.79%   91.97%   +0.18%     
==========================================
  Files           4        4              
  Lines         195      162      -33     
==========================================
- Hits          179      149      -30     
+ Misses         13       10       -3     
  Partials        3        3              

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

Copy link
Contributor

@muhlemmer muhlemmer left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This is a great contribution. I've left a few comments that will need to be resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using the exported json file, the import fails with:

go run main.go data import --api=localhost:8080 --insecure --issuer=http://localhost:8080 --key=275218352408933106.json --data=export.json
2024/07/09 17:29:23 failed to unmarshal data: proto: (line 1:2): unknown field "orgs"
exit status 1

That's because the Request has a Oneof field that has to be populated first. I'll make a suggestion below in the file.

Comment on lines +68 to +77
var req pb.ImportDataRequest

err = protojson.Unmarshal(data, &req)

if err != nil {
log.Fatalln("failed to unmarshal data:", err)
return
}

resp, err := client.ImportData(context.Background(), &req)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the propper message type for unmarshall like this:

Suggested change
var req pb.ImportDataRequest
err = protojson.Unmarshal(data, &req)
if err != nil {
log.Fatalln("failed to unmarshal data:", err)
return
}
resp, err := client.ImportData(context.Background(), &req)
var reqOrgs pb.ImportDataOrg
err = protojson.Unmarshal(data, &reqOrgs)
if err != nil {
log.Fatalln("failed to unmarshal data:", err)
return
}
resp, err := client.ImportData(context.Background(), &pb.ImportDataRequest{
Data: &pb.ImportDataRequest_DataOrgs{
DataOrgs: &reqOrgs,
},
Timeout: "5m", // TODO: from command line argument!
})

Timeout is a required field and will need to be parsed from the command line arguments as well.

Comment on lines +16 to +18
issuer := Cmd.PersistentFlags().String("issuer", "", "issuer of your ZITADEL instance (in the form: https://<instance>.zitadel.cloud or https://<yourdomain>)")
api := Cmd.PersistentFlags().String("api", "", "gRPC endpoint of your ZITADEL instance (in the form: <instance>.zitadel.cloud:443 or <yourdomain>:443)")
insecure := Cmd.PersistentFlags().Bool("insecure", false, "disable TLS to connect to gRPC API (use for local development only)")
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that these 3 command line options are quite redundant.

For example:

  1. If the issuer is http://localhost:8080, you already know the API endpoint is localhost:8080 and the connection is insecure.
  2. If the issuer is https://<instance>.zitadel.cloud, you already know the API endpoint is <instance>.zitadel.cloud:443 and the connection is secure.

For this example you'll need to parse the URL of the issuer and imply the the API configuration from there.

The reverse can also be applied:

  1. A --host flag can point to the API and will become part of the issuer
  2. The --insecure flag toggles https to http in front of the issuer. The API port toggles between 443 and 80 by default.
  3. A --port flag sets / overrides the port suffix to the host for the issuer and API.

In the last example, if one would only set <instance>.zitadel.cloud, the default will create issuer https://<instance>.zitadel.cloud and API endpoint <instance>.zitadel.cloud:443.

For user friendlyness I would want to stick to one of the above solutions, you may pick one.

@muhlemmer
Copy link
Contributor

Hi @yann-soubeyrand, do you intend to continue work on this PR?

@yann-soubeyrand
Copy link
Author

Hi @muhlemmer, sorry for the late reply. Yes, I’d be glad to work on this again, but I’ve just no time for it for the moment. My first intention when I opened this PR was to share the code which I used in a project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

3 participants