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

✨ feature: List artifacts + inspect individual artifacts #22

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

queer
Copy link
Contributor

@queer queer commented Mar 23, 2023

image

image

@queer queer requested a review from coryodaniel as a code owner March 23, 2023 02:30
Comment on lines 1 to 3
# artifact command help docs

TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure how to best fill this out

Comment on lines +72 to +73
func GetArtifact(client graphql.Client, orgID string, artifactID string) (*Artifact, error) {
// TODO(amy): The backend needs to permit service accounts to run this query!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haaaaaaaaate it and should/probably-will follow up w/ a PR for this

Copy link
Member

Choose a reason for hiding this comment

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

There’s nothing more permanent than a temporary solution ;)

Comment on lines +116 to +151

query getAllArtifacts($organizationId: ID!) {
artifacts(organizationId: $organizationId, input: {}) {
next
items {
id
name
type
data
artifactDefinition {
id
name
schema
url
}
}
}
}

query getAllArtifactsPaginated($organizationId: ID!, $after: String!) {
artifacts(organizationId: $organizationId, input: {after: $after}) {
next
items {
id
name
type
data
artifactDefinition {
id
name
schema
url
}
}
}
}
Copy link
Contributor Author

@queer queer Mar 23, 2023

Choose a reason for hiding this comment

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

Awkward and I hate it, but I can't pass a nil string when the query is getAllArtifacts($orgId: ID!, $after: String)? Maybe I'm just not used to Go's type system...

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I follow. Is the backend typed not nil? I see a “!” In the Gql

you’ll need a string pointer to pass nil or pass a “nilish” string “”

Copy link
Member

@coryodaniel coryodaniel left a comment

Choose a reason for hiding this comment

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

Just some changes around consistentency

var artifactInfoCommand = &cobra.Command{
Use: "info <artifact id>",
Aliases: []string{"i"},
Short: "Get information about an artifact",
Copy link
Member

Choose a reason for hiding this comment

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

I’d like to keep the CLI verb oriented get/list/create

func runListArtifacts(cmd *cobra.Command, args []string) error {
c := config.Get()
client := api.NewClient(c.URL, c.APIKey)
artifacts, err := api.GetAllArtifacts(client, c.OrgID)
Copy link
Member

Choose a reason for hiding this comment

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

Push all logic and presentation down into commands/

return err
}

fmt.Println("Artifacts:")
Copy link
Member

Choose a reason for hiding this comment

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

Use a bubble tea table for presenting a table of items

https://github.com/Evertras/bubble-table

func runArtifactInfo(cmd *cobra.Command, args []string) error {
c := config.Get()
client := api.NewClient(c.URL, c.APIKey)
artifact, err := api.GetArtifact(client, c.OrgID, args[0])
Copy link
Member

Choose a reason for hiding this comment

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

Same notes as above regarding command and bubble tea. We are trying to keep the cobra command as soon as possible and encapsulate all of the display logic.

ID string
Type string
ArtifactDefinition ArtifactDefinition
Data interface{}
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is a map[string]interface{} since it’s JSON data

Comment on lines +72 to +73
func GetArtifact(client graphql.Client, orgID string, artifactID string) (*Artifact, error) {
// TODO(amy): The backend needs to permit service accounts to run this query!
Copy link
Member

Choose a reason for hiding this comment

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

There’s nothing more permanent than a temporary solution ;)

{"massdriver/azure-service-principal"},
{"massdriver/gcp-service-account"},
{"massdriver/kubernetes-cluster"},
{"massdriver/aws-iam-role", "AWS IAM Role", ""},
Copy link
Member

Choose a reason for hiding this comment

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

Why are these changing? This is specifically for integrating credentials definitions not all artifacts … we also have a mediocre inflector that does this. If we need it elsewhere we should bring it up from the arttdef table UI component as a strict function Titleize()

but also still curious why credential struct being modified wrt listing all artifacts

id
name
type
data
Copy link
Member

Choose a reason for hiding this comment

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

Suspect re: data here. @xpositivityx are we auditing this field access?

Comment on lines +116 to +151

query getAllArtifacts($organizationId: ID!) {
artifacts(organizationId: $organizationId, input: {}) {
next
items {
id
name
type
data
artifactDefinition {
id
name
schema
url
}
}
}
}

query getAllArtifactsPaginated($organizationId: ID!, $after: String!) {
artifacts(organizationId: $organizationId, input: {after: $after}) {
next
items {
id
name
type
data
artifactDefinition {
id
name
schema
url
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I follow. Is the backend typed not nil? I see a “!” In the Gql

you’ll need a string pointer to pass nil or pass a “nilish” string “”

"github.com/massdriver-cloud/mass/internal/api"
)

func GetAllArtifacts(client graphql.Client, orgID string, name string) ([]api.Artifact, error) {
Copy link
Member

Choose a reason for hiding this comment

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

ListArtifacts to stay consistent

logic in your cobra commands should be moved here + tests

Goal is to make commands/ testable versions of the cobra commands but without any terminal I/O flag parsing outputting to files etc

@queer queer marked this pull request as draft March 27, 2023 17:28
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.

None yet

2 participants