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

Add logs to allowed-[repositories|plugins] #2810

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

jakubm-canva
Copy link
Contributor

Description

A handy log line, which supports a process of enabling allowed repo and plugins on buildkite-agent.

Context

My first attempt of enabling buildkite allowed-repo and allowed-plugins failed. I'm not entirely sure if I'm propagating the variables correctly, so this log line would be pretty useful in debugging.

Changes

Testing

  • Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
  • Code is formatted (with go fmt ./...)

## Context

A handy log line, which supports a process of enabling allowed repo and plugins on buildkite-agent.

## Intent

My first attempt of enabling buildkite allowed-repo and allowed-plugins failed. I'm not entirely sure if I'm propagating the variables correctly, so this log line would be pretty useful in debugging.
Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

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

Hi @jakubm-canva, thanks for the PR! I have a suggestion that might make the new logs slightly more useful, WDYT?

@@ -1032,6 +1032,7 @@ var AgentStartCommand = cli.Command{
}

if len(cfg.AllowedRepositories) > 0 {
l.Info("Evaluating allowed repositories has been enabled")
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if these would be better at the end of the block (after they've all been parsed - if any of them fail there will be a Fatal log), and if they included the values (after parsing) so you know what it understood, e.g.

l.Info("Allowed repositories patterns: %q", agentConf.AllowedRepositories)
...
l.Info("Allowed plugins patterns: %q", agentConf.AllowedPlugins)

Copy link
Contributor Author

@jakubm-canva jakubm-canva Jun 4, 2024

Choose a reason for hiding this comment

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

Sounds good to me. Let me rework it and update the PR.

FYI. I recall you from SYD-ODI. You were working with Marek 😉 He was my 🇵🇱 connection.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! Hey 👋 I thought you looked familiar. I do miss my old colleagues a bit 😔 How long have you been at Canva?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After nearly 7 years in network engineering and 1.5 year as embedded SWE in ChromeOS, the time has come to make a change. Working on Infra / CI since Apr, 2023 and I'm having good fun 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, glad you're having fun! I took some time off between exiting SRE and coming to BK in Oct 2022, it was worth it.

@DrJosh9000 DrJosh9000 enabled auto-merge June 5, 2024 00:58
@DrJosh9000 DrJosh9000 merged commit cc60fa4 into buildkite:main Jun 5, 2024
1 check passed
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