-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[TT-1981] use chainlink cli in the keystone smoke test #16146
Conversation
…one-por-test-broken
…n that sets GH_TOKEN as env var available to tests
@@ -44,6 +44,13 @@ The test requires several environment variables. Below is a launch configuration | |||
- **`GITHUB_READ_TOKEN`**: Required for downloading the `cron` capability binary and `chainlink-cli` (if enabled). Requires `content:read` permission for `smartcontractkit/capabilities` and `smartcontractkit/dev-platform` repositories. Use a fine-grained personal access token (PAT) tied to the **organization’s GitHub account**. | |||
- **`GIST_WRITE_TOKEN`**: Required only for compiling and uploading a new workflow. It needs `gist:read:write` permissions and should be a fine-grained PAT **tied to your personal GitHub account**. | |||
|
|||
Test also expects you to have the Job Distributor image available locally. By default, `environment.toml` expectes image tagged as `jd-test-1:latest`. The easiest way to get it, is to clone the Job Distributor repository and build it locally with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit/ typo expectes -> expects
&oauth2.Token{AccessToken: ghToken}, | ||
) | ||
tc := oauth2.NewClient(ctx, ts) | ||
ctx := context.Background() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be some reasonable timeout? a few minutes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, yes
if err != nil { | ||
return content, errors.Wrapf(err, "failed to list releases for %s", repository) | ||
} | ||
ghReleases, _, err := ghClient.Repositories.ListReleases(context.Background(), owner, repository, &github.ListOptions{PerPage: 20}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use ctx
var
} | ||
asset, _, err := ghClient.Repositories.DownloadReleaseAsset(context.Background(), owner, repository, assetID, tc) | ||
if err != nil { | ||
return content, errors.Wrapf(err, "failed to download asset %s for %s", assetName, *ghRelease.TagName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/errors is depreciated
use fmt.Errorf("this is a wrapped error: %w", err)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, but we keep using it as there's no good replacement. fmt.Errorf
doesn't give you the stacktrace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm. ok.
we should need the stack trace if the error wrapping is done well
we probably do return err
in too many places and lose the meaning of the err along the way
if err != nil { | ||
return err | ||
return errors.Wrapf(err, "failed to download chainlink-cli asset %s", chainlinkCliAssetFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'%w'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only if we go with fmt.Errof
, which I'd rather avoid.
|
||
cmd := exec.Command("tar", "-xvf", tmpfile.Name(), "-C", ".") // #nosec G204 | ||
if cmd.Run() != nil { | ||
return errors.Wrapf(err, "failed to extract chainlink-cli asset %s", chainlinkCliAssetFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
extractedFileName := fmt.Sprintf("cre_%s_%s_%s", version, system, arch) | ||
cmd = exec.Command("chmod", "+x", extractedFileName) | ||
if cmd.Run() != nil { | ||
return errors.Wrapf(err, "failed to make %s executable", extractedFileName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
if err != nil { | ||
return err | ||
return errors.Wrapf(err, "failed to open %s", extractedFileName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
if err != nil { | ||
return err | ||
return errors.Wrapf(err, "failed to get absolute path for %s", tmpfile.Name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
@@ -712,7 +748,7 @@ func compileWorkflowWithChainlinkCli(t *testing.T, in *WorkflowTestConfig, feeds | |||
|
|||
var outputBuffer bytes.Buffer | |||
|
|||
compileCmd := exec.Command("chainlink-cli", "workflow", "compile", "-S", settingsFile.Name(), "-c", configFile.Name(), "main.go") // #nosec G204 | |||
compileCmd := exec.Command(chainlinkCliCommand, "workflow", "compile", "-S", settingsFile.Name(), "-c", configFile.Name(), "main.go") // #nosec G204 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this work from all current working dirs? hardcoded "main.go" looks suspicious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't run in current working directory, but in:
compileCmd.Dir = *in.WorkflowConfig.ChainlinkCLI.FolderLocation
main.go
is hardcoded, because that's the has been the typical name of entry workflow file so far. Also, chainlink-cli
expects you to have go.mod
in the workflow folder, which won't allow you to compile workflow that resides in a package different than main. I'll add a comment.
|
disclaimer: idk why we see such a long commit history, it's completely out of whack. This PR adds but 4-5 new commits.
changes: