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: multi instance support #178

Merged
merged 24 commits into from
Sep 22, 2023

Conversation

TobiaszCudnik
Copy link
Contributor

@TobiaszCudnik TobiaszCudnik commented Sep 15, 2023

Because

  • user can have more than 1 instance of Instill AI

This commit

  • adds instance management and switching

Changes

  • instances management
    • add, edit, list, remove, set-default
  • markdown tables
  • tests
  • typed config (for hosts)
  • default config when empty
  • hostname validation
  • breaking dependabot updates
  • readme

@linear
Copy link

linear bot commented Sep 15, 2023

INS-1661 CLI: multi-instance support

  • add / remove / select a target host
  • integrate with auth
    • oauth hostname differs from the API hostname (for the same instance)
  • aggregate results from >1 source
    • eg --local --cloud --all
    • ability to select --all as a default
# examin existing instances
$ inst instances list
+--------------+-------------------+-------------------+----------------------------+
| Type         | API Hostname      | Oauth2 Hostname   | Issuer                     |
+--------------+-------------------+-------------------+----------------------------+
| cloud        | api.instill.tech  | auth.instill.tech | https://auth.instill.tech/ |
+--------------+-------------------+-------------------+----------------------------+
| local        | instill.localhost |                   |                            |
+--------------+-------------------+-------------------+----------------------------+

# set the local instance (http)
$ inst instances set local instill.localhost

# set the cloud instance (https)
$ inst instances set cloud api.instill.tech \
		--oauth auth.instill.tech \
		--issuer https://auth.instill.tech/

Later

  • support for >1 local instance
  • support for >1 cloud instance

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Patch coverage: 54.84% and project coverage change: +0.96% 🎉

Comparison is base (b9e1db9) 50.13% compared to head (f23e835) 51.09%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
+ Coverage   50.13%   51.09%   +0.96%     
==========================================
  Files          48       54       +6     
  Lines        3700     4196     +496     
==========================================
+ Hits         1855     2144     +289     
- Misses       1674     1833     +159     
- Partials      171      219      +48     
Flag Coverage Δ
unittests 51.09% <54.84%> (+0.96%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
internal/config/config_file.go 57.60% <ø> (ø)
internal/config/config_type.go 96.07% <ø> (ø)
pkg/cmd/auth/status/status.go 28.57% <0.00%> (ø)
pkg/cmd/root/help_topic.go 84.61% <ø> (ø)
pkg/cmd/root/root.go 0.00% <0.00%> (ø)
pkg/cmdutil/errors.go 26.31% <ø> (ø)
pkg/cmdutil/output.go 0.00% <0.00%> (ø)
pkg/cmd/auth/login/login.go 42.30% <18.51%> (-2.14%) ⬇️
internal/config/from_file.go 53.17% <33.58%> (-11.27%) ⬇️
pkg/cmd/instances/instances.go 40.74% <40.74%> (ø)
... and 15 more

... and 1 file with indirect coverage changes

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

internal/config/stub.go Fixed Show fixed Hide fixed
Signed-off-by: Tobias Cudnik <[email protected]>

fix GH workflows

Signed-off-by: Tobias Cudnik <[email protected]>

fix GH workflows

Signed-off-by: Tobias Cudnik <[email protected]>

fix GH workflows

Signed-off-by: Tobias Cudnik <[email protected]>

fix GH workflows

Signed-off-by: Tobias Cudnik <[email protected]>

fix GH workflows

Signed-off-by: Tobias Cudnik <[email protected]>

ci: fix workflows

- missing auth0 vars - .env for local dev - aligned build system - instill-dev.eu tenant working

Signed-off-by: Tobias Cudnik <[email protected]>

update go/mods

Signed-off-by: Tobias Cudnik <[email protected]>
- wip: list cmd
- table renderer
- markdown output

Signed-off-by: Tobias Cudnik <[email protected]>
- typed host config
- list cmd
- add cmd

Signed-off-by: Tobias Cudnik <[email protected]>
- edit cmd
- remove cmd
- save config

Signed-off-by: Tobias Cudnik <[email protected]>
- missing oauth fields
- bound auth to config
- default config on bootstrap

Signed-off-by: Tobias Cudnik <[email protected]>
Signed-off-by: Tobias Cudnik <[email protected]>
Signed-off-by: Tobias Cudnik <[email protected]>
- fixed default hostname
- fixed table urls
- params validation

Signed-off-by: Tobias Cudnik <[email protected]>
- `api` config integration

Signed-off-by: Tobias Cudnik <[email protected]>
Signed-off-by: Tobias Cudnik <[email protected]>
Signed-off-by: Tobias Cudnik <[email protected]>
@TobiaszCudnik TobiaszCudnik force-pushed the tobias/ins-1661-cli-multi-instance-support branch from dbfb605 to f32830e Compare September 15, 2023 13:17
Signed-off-by: Tobias Cudnik <[email protected]>
Signed-off-by: Tobias Cudnik <[email protected]>
- added `instances set-default`
- fixed partial edits
- re-fixed urls in tables (rebase)
- fixed hostname parsing
- dependabot updates

Signed-off-by: Tobias Cudnik <[email protected]>
Signed-off-by: Tobias Cudnik <[email protected]>
Signed-off-by: Tobias Cudnik <[email protected]>
internal/config/stub.go Fixed Show fixed Hide fixed
Signed-off-by: Tobias Cudnik <[email protected]>
Signed-off-by: Tobias Cudnik <[email protected]>
Signed-off-by: Tobias Cudnik <[email protected]>
Signed-off-by: Tobias Cudnik <[email protected]>
@TobiaszCudnik TobiaszCudnik marked this pull request as ready for review September 20, 2023 11:00
Copy link
Member

@pinglin pinglin left a comment

Choose a reason for hiding this comment

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

Please see the comments and suggested changes.

cmd/instill/main.go Outdated Show resolved Hide resolved
internal/config/stub.go Show resolved Hide resolved
internal/config/stub.go Outdated Show resolved Hide resolved
internal/config/stub.go Outdated Show resolved Hide resolved
internal/instance/host.go Outdated Show resolved Hide resolved
internal/config/stub.go Outdated Show resolved Hide resolved
internal/oauth2/auth_code_flow.go Outdated Show resolved Hide resolved
pkg/cmd/auth/login/login.go Outdated Show resolved Hide resolved
pkg/cmd/instances/add_test.go Outdated Show resolved Hide resolved
pkg/cmd/instances/edit_test.go Outdated Show resolved Hide resolved
- imports
- renames

Signed-off-by: Tobias Cudnik <[email protected]>
Oauth2Hostname: "auth.instill.tech",
Oauth2Audience: "https://api.instill.tech",
Oauth2Issuer: "https://auth.instill.tech/",
Oauth2ClientSecret: "foobar",

Check failure

Code scanning / CodeQL

Hard-coded credentials

Hard-coded [secret](1).
Signed-off-by: Tobias Cudnik <[email protected]>
@TobiaszCudnik
Copy link
Contributor Author

Please see the comments and suggested changes.

@pinglin Ive implemented the renames and formatted imports. Im happy theres no concerns regarding the code itself. We should incorporate goimports into the pipeline.

@pinglin
Copy link
Member

pinglin commented Sep 22, 2023

Please see the comments and suggested changes.

@pinglin Ive implemented the renames and formatted imports. Im happy theres no concerns regarding the code itself. We should incorporate goimports into the pipeline.

Yeah, that will be very ideal. @praharshjain has proposed and tried it before actually. Any guideline for this?

@TobiaszCudnik
Copy link
Contributor Author

This seems to be the right source:
https://github.com/dnephin/pre-commit-golang

Import order should be linted, like any other required conventions. AFAIK we're using the default golangci-lint config.

Signed-off-by: Tobias Cudnik <[email protected]>
Signed-off-by: Tobias Cudnik <[email protected]>
Copy link
Member

@pinglin pinglin left a comment

Choose a reason for hiding this comment

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

LGTM

@TobiaszCudnik TobiaszCudnik merged commit d683002 into main Sep 22, 2023
@TobiaszCudnik TobiaszCudnik deleted the tobias/ins-1661-cli-multi-instance-support branch September 22, 2023 17:09
TobiaszCudnik pushed a commit that referenced this pull request Sep 27, 2023
Because

- user can have more than 1 instance of Instill AI

This commit

- adds instance management and switching

- instances management
  - `add`, `edit`, `list`, `remove`, `set-default`
- markdown tables
- tests
- typed config (for hosts)
- default config when empty
- hostname validation
- breaking dependabot updates
- readme

---------

Signed-off-by: Tobias Cudnik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: 👋 Done
Development

Successfully merging this pull request may close these issues.

3 participants