Skip to content

Conversation

@jfairchild
Copy link

@jfairchild jfairchild commented Jun 11, 2025

If this is a change to the core functionality, did you make a corresponding PR to drone-eks?

  • yes
  • no
  • n/a

Did you update the tests?

  • yes
  • no
  • n/a

Did you update the docs?

  • yes
  • no
  • n/a

@jfairchild jfairchild requested a review from a team as a code owner June 11, 2025 15:37
@jfairchild jfairchild enabled auto-merge (squash) June 11, 2025 16:36
Copy link
Member

@tonglil tonglil left a comment

Choose a reason for hiding this comment

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

Thanks @jfairchild !

Comment on lines +79 to +80
webhook: ${{ secrets.SLACK_WEBHOOK_URL }}
webhook-type: incoming-webhook
Copy link
Member

Choose a reason for hiding this comment

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

The current CI is failing with this.
I'm not aware of the updated syntax, but we can give this a try.

Comment on lines +12 to +14
find ${gcloud_sdk_bin_dir} -type f -name ${extra_kubectl_glob} -exec basename {} \; |
cut -d '.' -f 2,3 |
sort -g
Copy link
Member

Choose a reason for hiding this comment

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

Does this still work?

rev = "[unknown]"
}

fmt.Printf("Drone GKE Plugin built from %s\n", rev)
Copy link
Member

Choose a reason for hiding this comment

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

If I recall, for these messages to render properly in Drone, the newlines are necessary.

log("Parsing Project ID from credentials")
project = getProjectFromToken(token)
if project == "" {
return fmt.Errorf("Missing required param: project")
Copy link
Member

Choose a reason for hiding this comment

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

And same with capitalizing the error message - it's not idiomatic go, but it's used as output in the logs.

Copy link
Member

Choose a reason for hiding this comment

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

There's valid changes in here that I'll extract out, but will revert the commented on changes.

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.

2 participants