- Notifications
You must be signed in to change notification settings - Fork921
fix: improve log on provisioner daemon started with pk#15588
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
995a0462ca6c91c67d322911a47d1a019bbc629f0754437f28eeffb1960084d3aa81edebcf6870cff661d4472a2ee0fef6703668b9e0a2d3b5ff4657ea193f08ab032File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -104,6 +104,22 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command { | ||
| return err | ||
| } | ||
| displayedTags := make(map[string]string, len(tags)) | ||
| if provisionerKey != "" { | ||
| pkDetails, err := client.GetProvisionerKey(ctx, provisionerKey) | ||
| if err != nil { | ||
| return xerrors.New("unable to get provisioner key details") | ||
defelmnq marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| } | ||
| for k, v := range pkDetails.Tags { | ||
| displayedTags[k] = v | ||
| } | ||
Comment on lines +114 to +116 Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I think we can overwrite There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I preferred to keep two different variables as otherwise it would require some other logic changes. The tag variable is used to call the wdyt ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. For the sake of correctness, I'd like to make this change. | ||
| } else { | ||
| for key, val := range tags { | ||
| displayedTags[key] = val | ||
| } | ||
| } | ||
| if name == "" { | ||
| name = cliutil.Hostname() | ||
| } | ||
| @@ -131,7 +147,7 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command { | ||
| defer closeLogger() | ||
| } | ||
| if len(displayedTags) == 0 { | ||
| logger.Info(ctx, "note: untagged provisioners can only pick up jobs from untagged templates") | ||
| } | ||
| @@ -202,7 +218,7 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command { | ||
| defer closeFunc() | ||
| } | ||
| logger.Info(ctx, "starting provisioner daemon", slog.F("tags",displayedTags), slog.F("name", name)) | ||
| connector := provisionerd.LocalProvisioners{ | ||
| string(database.ProvisionerTypeTerraform): proto.NewDRPCProvisionerClient(terraformClient), | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -294,6 +294,73 @@ func TestProvisionerDaemon_ProvisionerKey(t *testing.T) { | ||
| require.Equal(t, proto.CurrentVersion.String(), daemons[0].APIVersion) | ||
| }) | ||
| t.Run("OKWithTags", func(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Are there any sad-cases that we could be testing here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. We could add a test to validate what happen in case of the Otherwise the logic should not be impacted and the endpoint itself seems pretty much well tested. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I'm not really understanding the point of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Yeah that's exactly it - I am open to the behavior we want to apply here but globally if we can not contact the backend / retrieve the key , I returned and error and did not continued to start the provisioner. We can instead just output an error and continue as if nothing happened - but the log will be the same as of now (without the tags) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Cool, no need to change the behaviour - I was just confirming what the intent was for this test. | ||
| t.Parallel() | ||
| ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) | ||
| defer cancel() | ||
| client, user := coderdenttest.New(t, &coderdenttest.Options{ | ||
| ProvisionerDaemonPSK: "provisionersftw", | ||
| LicenseOptions: &coderdenttest.LicenseOptions{ | ||
| Features: license.Features{ | ||
| codersdk.FeatureExternalProvisionerDaemons: 1, | ||
| codersdk.FeatureMultipleOrganizations: 1, | ||
| }, | ||
| }, | ||
| }) | ||
| //nolint:gocritic // ignore This client is operating as the owner user, which has unrestricted permissions | ||
| res, err := client.CreateProvisionerKey(ctx, user.OrganizationID, codersdk.CreateProvisionerKeyRequest{ | ||
| Name: "dont-TEST-me", | ||
| Tags: map[string]string{ | ||
| "tag1": "value1", | ||
| "tag2": "value2", | ||
| }, | ||
| }) | ||
| require.NoError(t, err) | ||
| inv, conf := newCLI(t, "provisionerd", "start", "--key", res.Key, "--name=matt-daemon") | ||
| err = conf.URL().Write(client.URL.String()) | ||
| require.NoError(t, err) | ||
| pty := ptytest.New(t).Attach(inv) | ||
| clitest.Start(t, inv) | ||
| pty.ExpectNoMatchBefore(ctx, "check entitlement", "starting provisioner daemon") | ||
| pty.ExpectMatchContext(ctx, `tags={"tag1":"value1","tag2":"value2"}`) | ||
| var daemons []codersdk.ProvisionerDaemon | ||
| require.Eventually(t, func() bool { | ||
| daemons, err = client.OrganizationProvisionerDaemons(ctx, user.OrganizationID, nil) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return len(daemons) == 1 | ||
| }, testutil.WaitShort, testutil.IntervalSlow) | ||
| require.Equal(t, "matt-daemon", daemons[0].Name) | ||
| require.Equal(t, provisionersdk.ScopeOrganization, daemons[0].Tags[provisionersdk.TagScope]) | ||
| require.Equal(t, buildinfo.Version(), daemons[0].Version) | ||
| require.Equal(t, proto.CurrentVersion.String(), daemons[0].APIVersion) | ||
| }) | ||
| t.Run("NoProvisionerKeyFound", func(t *testing.T) { | ||
| t.Parallel() | ||
| ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) | ||
| defer cancel() | ||
| client, _ := coderdenttest.New(t, &coderdenttest.Options{ | ||
| ProvisionerDaemonPSK: "provisionersftw", | ||
| LicenseOptions: &coderdenttest.LicenseOptions{ | ||
| Features: license.Features{ | ||
| codersdk.FeatureExternalProvisionerDaemons: 1, | ||
| codersdk.FeatureMultipleOrganizations: 1, | ||
| }, | ||
| }, | ||
| }) | ||
| inv, conf := newCLI(t, "provisionerd", "start", "--key", "ThisKeyDoesNotExist", "--name=matt-daemon") | ||
| err := conf.URL().Write(client.URL.String()) | ||
| require.NoError(t, err) | ||
| err = inv.WithContext(ctx).Run() | ||
| require.ErrorContains(t, err, "unable to get provisioner key details") | ||
| }) | ||
| t.Run("NoPSK", func(t *testing.T) { | ||
| t.Parallel() | ||
Uh oh!
There was an error while loading.Please reload this page.