Ext 283 remove legacy org slug #717
Conversation
Codecov Report
@@ Coverage Diff @@
## master #717 +/- ##
==========================================
- Coverage 28.82% 28.50% -0.32%
==========================================
Files 42 42
Lines 4860 4925 +65
==========================================
+ Hits 1401 1404 +3
- Misses 3261 3323 +62
Partials 198 198
Continue to review full report at Codecov.
|
| @@ -17,6 +17,7 @@ func newLocalExecuteCommand(config *settings.Config) *cobra.Command { | |||
|
|
|||
| local.AddFlagsForDocumentation(buildCommand.Flags()) | |||
| buildCommand.Flags().StringP("org-slug", "o", "", "organization slug (for example: github/example-org), used when a config depends on private orbs belonging to that org") | |||
| buildCommand.Flags().String("org-id", "", "organization id, used when a config depends on private orbs belonging to that org") | |||
There was a problem hiding this comment.
nitpick here but I'd argue keeping the wording the same (or extract to a variable even) for the three <command>.Flags().String("org-id"...
There was a problem hiding this comment.
Could you give a full example? The wording is the same as far as I can see...
There was a problem hiding this comment.
in cmd/config.go we have:
processCommand.Flags().String("org-id", "", "organization id used when a config depends on private orbs belonging to that org")
validateCommand.Flags().String("org-id", "", "organization id used when a config depends on private orbs belonging to that org")in retrospect they're almost identical (just a comma that's different). Will leave it up to you if you think it's worth extracting to a variable.
|
few minors comments, aside from that LGTM! |
|
Hey @joeyorlando , this isnt ready for review. lol |
|
|
||
| // ConfigQuery calls the GQL API to validate and process config with the org id | ||
| func ConfigQuery(cl *graphql.Client, configPath string, orgId string, params pipeline.Parameters, values pipeline.Values) (*ConfigResponse, error) { |
There was a problem hiding this comment.
Nitpick so optional, but I think we're capitalizing ID in the rest of the project, so orgID.
| // ConfigQuery calls the GQL API to validate and process config | ||
| func ConfigQuery(cl *graphql.Client, configPath string, orgSlug string, params pipeline.Parameters, values pipeline.Values) (*ConfigResponse, error) { | ||
| // ConfigQueryLegacy calls the GQL API to validate and process config with the legacy orgSlug | ||
| func ConfigQueryLegacy(cl *graphql.Client, configPath string, orgSlug string, params pipeline.Parameters, values pipeline.Values) (*ConfigResponse, error) { |
There was a problem hiding this comment.
I think we can simplify these two functions (ConfigQuery, and ConfigQueryLegacy) into one. Since this is graph, couldn't we just send everything that we've collected, like both the org slug and orgID? The API should already be handling this logic so there's no need to do it here too.
There was a problem hiding this comment.
We tried that at first, remember? Graph did not like it when one of those fields wasn't sent. Keeping the methods separate will also allow us to remove the legacy code easier in the future. :)
There was a problem hiding this comment.
Really? I just tried refactoring it locally and it worked just fine? Not a big deal though but I think less code is better than more code. Let me know if you want to spend the time to refactor it and I can help ya out :D
Checklist
=========
Changes
=======
Considerations
==============
In case of users currently using the legacy slug org, it remains for now.