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.

Sign up for GitHub

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

Ext 283 remove legacy org slug #717

Merged
merged 5 commits into from
Jun 6, 2022
Merged

Ext 283 remove legacy org slug #717

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2c079f7
Org UUID support
corinnesollows May 30, 2022
d964ccc
Made two methods
corinnesollows May 30, 2022
cb03086
Test for OrgId
corinnesollows May 30, 2022
c8d7243
Merge branch 'master' into EXT-283-remove-legacy-org-slug
corinnesollows May 31, 2022
e9914b6
Comments
corinnesollows May 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 63 additions & 5 deletions api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,16 +513,14 @@ func WhoamiQuery(cl *graphql.Client) (*WhoamiResponse, error) {
return &response, nil
}

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@corinnesollows corinnesollows Jun 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

var response BuildConfigResponse
var query string

config, err := loadYaml(configPath)
if err != nil {
return nil, err
}

// GraphQL isn't forwards-compatible, so we are unusually selective here about
// passing only non-empty fields on to the API, to minimize user impact if the
// backend is out of date.
Expand All @@ -546,6 +544,7 @@ func ConfigQuery(cl *graphql.Client, configPath string, orgSlug string, params p

request := graphql.NewRequest(query)
request.Var("config", config)

if values != nil {
request.Var("pipelineValues", pipeline.PrepareForGraphQL(values))
}
Expand All @@ -556,17 +555,76 @@ func ConfigQuery(cl *graphql.Client, configPath string, orgSlug string, params p
}
request.Var("pipelineParametersJson", string(pipelineParameters))
}

if orgSlug != "" {
request.Var("orgSlug", orgSlug)
}

request.SetToken(cl.Token)

err = cl.Run(request, &response)

if err != nil {
return nil, errors.Wrap(err, "Unable to validate config")
}
if len(response.BuildConfig.ConfigResponse.Errors) > 0 {
return nil, &response.BuildConfig.ConfigResponse.Errors
}

return &response.BuildConfig.ConfigResponse, nil
}

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick so optional, but I think we're capitalizing ID in the rest of the project, so orgID.

var response BuildConfigResponse
var query string
config, err := loadYaml(configPath)
if err != nil {
return nil, err
}
// GraphQL isn't forwards-compatible, so we are unusually selective here about
// passing only non-empty fields on to the API, to minimize user impact if the
// backend is out of date.
var fieldAddendums string
if orgId != "" {
joeyorlando marked this conversation as resolved.
Show resolved Hide resolved
fieldAddendums += ", orgId: $orgId"
}
if len(params) > 0 {
fieldAddendums += ", pipelineParametersJson: $pipelineParametersJson"
}
query = fmt.Sprintf(
`query ValidateConfig ($config: String!, $pipelineParametersJson: String, $pipelineValues: [StringKeyVal!], $orgId: UUID!) {
buildConfig(configYaml: $config, pipelineValues: $pipelineValues%s) {
valid,
errors { message },
sourceYaml,
outputYaml
}
}`,
fieldAddendums)

request := graphql.NewRequest(query)
request.Var("config", config)

if values != nil {
request.Var("pipelineValues", pipeline.PrepareForGraphQL(values))
}
if params != nil {
pipelineParameters, err := json.Marshal(params)
if err != nil {
return nil, fmt.Errorf("unable to serialize pipeline values: %s", err.Error())
}
request.Var("pipelineParametersJson", string(pipelineParameters))
}

if orgId != "" {
request.Var("orgId", orgId)
}
request.SetToken(cl.Token)

err = cl.Run(request, &response)
if err != nil {
return nil, errors.Wrap(err, "Unable to validate config")
}
if len(response.BuildConfig.ConfigResponse.Errors) > 0 {
return nil, &response.BuildConfig.ConfigResponse.Errors
}
Expand Down
1 change: 1 addition & 0 deletions cmd/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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"...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you give a full example? The wording is the same as far as I can see...

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


return buildCommand
}
Expand Down
40 changes: 30 additions & 10 deletions cmd/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package cmd
import (
"fmt"
"io/ioutil"
"strings"

"github.com/CircleCI-Public/circleci-cli/api"
"github.com/CircleCI-Public/circleci-cli/api/graphql"
Expand Down Expand Up @@ -77,6 +78,7 @@ func newConfigCommand(config *settings.Config) *cobra.Command {
panic(err)
}
validateCommand.Flags().StringP("org-slug", "o", "", "organization slug (for example: github/example-org), 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")

processCommand := &cobra.Command{
Use: "process <path>",
Expand Down Expand Up @@ -121,6 +123,8 @@ func newConfigCommand(config *settings.Config) *cobra.Command {

// The <path> arg is actually optional, in order to support compatibility with the --path flag.
func validateConfig(opts configOptions, flags *pflag.FlagSet) error {
var err error
var response *api.ConfigResponse
path := local.DefaultConfigPath
// First, set the path to configPath set by --path flag for compatibility
if configPath != "" {
Expand All @@ -132,11 +136,18 @@ func validateConfig(opts configOptions, flags *pflag.FlagSet) error {
path = opts.args[0]
}

orgSlug, _ := flags.GetString("org-slug")

response, err := api.ConfigQuery(opts.cl, path, orgSlug, nil, pipeline.LocalPipelineValues())
if err != nil {
return err
orgID, _ := flags.GetString("org-id")
if strings.TrimSpace(orgID) != "" {
response, err = api.ConfigQuery(opts.cl, path, orgID, nil, pipeline.LocalPipelineValues())
if err != nil {
return err
}
} else {
orgSlug, _ := flags.GetString("org-slug")
response, err = api.ConfigQueryLegacy(opts.cl, path, orgSlug, nil, pipeline.LocalPipelineValues())
if err != nil {
return err
}
}

// check if a deprecated Linux VM image is being used
Expand All @@ -159,10 +170,10 @@ func validateConfig(opts configOptions, flags *pflag.FlagSet) error {
}

func processConfig(opts configOptions, flags *pflag.FlagSet) error {
orgSlug, _ := flags.GetString("org-slug")
paramsYaml, _ := flags.GetString("pipeline-parameters")

var response *api.ConfigResponse
var params pipeline.Parameters
var err error

if len(paramsYaml) > 0 {
// The 'src' value can be a filepath, or a yaml string. If the file cannot be read sucessfully,
Expand All @@ -178,9 +189,18 @@ func processConfig(opts configOptions, flags *pflag.FlagSet) error {
}
}

response, err := api.ConfigQuery(opts.cl, opts.args[0], orgSlug, params, pipeline.LocalPipelineValues())
if err != nil {
return err
orgID, _ := flags.GetString("org-id")
if strings.TrimSpace(orgID) != "" {
response, err = api.ConfigQuery(opts.cl, opts.args[0], orgID, params, pipeline.LocalPipelineValues())
if err != nil {
return err
}
} else {
orgSlug, _ := flags.GetString("org-slug")
response, err = api.ConfigQueryLegacy(opts.cl, opts.args[0], orgSlug, params, pipeline.LocalPipelineValues())
if err != nil {
return err
}
}

fmt.Print(response.OutputYaml)
Expand Down
62 changes: 62 additions & 0 deletions cmd/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,68 @@ var _ = Describe("Config", func() {
})

Describe("validating configs with private orbs", func() {
config := "version: 2.1"
orgId := "bb604b45-b6b0-4b81-ad80-796f15eddf87"
var expReq string

BeforeEach(func() {
command = exec.Command(pathCLI,
"config", "validate",
"--skip-update-check",
"--token", token,
"--host", tempSettings.TestServer.URL(),
"--org-id", orgId,
"-",
)

stdin, err := command.StdinPipe()
Expect(err).ToNot(HaveOccurred())
_, err = io.WriteString(stdin, config)
Expect(err).ToNot(HaveOccurred())
stdin.Close()

query := `query ValidateConfig ($config: String!, $pipelineParametersJson: String, $pipelineValues: [StringKeyVal!], $orgId: UUID!) {
buildConfig(configYaml: $config, pipelineValues: $pipelineValues, orgId: $orgId) {
valid,
errors { message },
sourceYaml,
outputYaml
}
}`

r := graphql.NewRequest(query)
r.Variables["config"] = config
r.Variables["orgId"] = orgId
r.Variables["pipelineValues"] = pipeline.PrepareForGraphQL(pipeline.LocalPipelineValues())

req, err := r.Encode()
Expect(err).ShouldNot(HaveOccurred())
expReq = req.String()
})

It("returns an error when validating a config with a private orb", func() {
expResp := `{
"buildConfig": {
"errors": [
{"message": "permission denied"}
]
}
}`

tempSettings.AppendPostHandler(token, clitest.MockRequestResponse{
Status: http.StatusOK,
Request: expReq,
Response: expResp,
})

session, err := gexec.Start(command, GinkgoWriter, GinkgoWriter)
Expect(err).ShouldNot(HaveOccurred())
Eventually(session.Err, time.Second*3).Should(gbytes.Say("Error: permission denied"))
Eventually(session).Should(clitest.ShouldFail())
})
})

Describe("validating configs with private orbs Legacy", func() {
config := "version: 2.1"
orgSlug := "circleci"
var expReq string
Expand Down
24 changes: 18 additions & 6 deletions local/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"os"
"os/exec"
"regexp"
"strings"
"syscall"

"github.com/CircleCI-Public/circleci-cli/api"
Expand All @@ -22,13 +23,24 @@ var picardRepo = "circleci/picard"
const DefaultConfigPath = ".circleci/config.yml"

func Execute(flags *pflag.FlagSet, cfg *settings.Config) error {
processedArgs, configPath := buildAgentArguments(flags)
orgSlug, _ := flags.GetString("org-slug")
var err error
var configResponse *api.ConfigResponse
cl := graphql.NewClient(cfg.HTTPClient, cfg.Host, cfg.Endpoint, cfg.Token, cfg.Debug)
configResponse, err := api.ConfigQuery(cl, configPath, orgSlug, nil, pipeline.LocalPipelineValues())

if err != nil {
return err
processedArgs, configPath := buildAgentArguments(flags)

orgID, _ := flags.GetString("org-id")
joeyorlando marked this conversation as resolved.
Show resolved Hide resolved
if strings.TrimSpace(orgID) != "" {
configResponse, err = api.ConfigQuery(cl, configPath, orgID, nil, pipeline.LocalPipelineValues())
if err != nil {
return err
}
} else {
orgSlug, _ := flags.GetString("org-slug")
configResponse, err = api.ConfigQueryLegacy(cl, configPath, orgSlug, nil, pipeline.LocalPipelineValues())
if err != nil {
return err
}
}

if !configResponse.Valid {
Expand Down Expand Up @@ -118,7 +130,7 @@ func buildAgentArguments(flags *pflag.FlagSet) ([]string, string) {

// build a list of all supplied flags, that we will pass on to build-agent
flags.Visit(func(flag *pflag.Flag) {
if flag.Name != "org-slug" && flag.Name != "config" && flag.Name != "debug" {
if flag.Name != "org-slug" && flag.Name != "config" && flag.Name != "debug" && flag.Name != "org-id" {
result = append(result, unparseFlag(flags, flag)...)
}
})
Expand Down