-
-
Notifications
You must be signed in to change notification settings - Fork 378
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 PULLREQUEST_DRONE_PULL_REQUEST drone env #3939
Add PULLREQUEST_DRONE_PULL_REQUEST drone env #3939
Conversation
@6543 yeah, that env var is definitely the setting I use to get this particular thing to work |
@wez does the server run on x86 ? |
I run this particular configuration on amd64 and aarch64; so intel and arm, but 64-bit only |
For clarity, I have an amd64 agent and an aarch64 agent. They both build their respective platform packages and docker image, and they both have a step that tries to merge the other architectures together into a multiarch image with |
Well in this case the server image |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3939 +/- ##
==========================================
+ Coverage 26.80% 26.83% +0.02%
==========================================
Files 394 394
Lines 27496 27507 +11
==========================================
+ Hits 7370 7381 +11
Misses 19425 19425
Partials 701 701 ☔ View full report in Codecov by Sentry. |
Thanks; I appreciate you doing this, but I'm not sure if I'll be able to try that out today ahead of this being merged as I had to roll back to 2.6 (and reinstall/reconfigure, because there isn't a backwards db migration!) and I had to unblock some other work for today. |
Tearing down https://woodpecker-ci-woodpecker-pr-3939.surge.sh |
This reverts commit 401df30. The docker merge step is failing: ``` envconfig.Process: assigning PULLREQUEST_DRONE_PULL_REQUEST to Number: converting '' to type int. details: strconv.ParseInt: parsing "": invalid syntax ``` It seems that the compatibility change in woodpecker-ci/woodpecker#3939 isn't sufficient. Let's see if we can still run with the environment set manually, or if we'll need to blackjack our own woodpecker plugin for this.
FWIW, I got around to upgrading to 2.7.1 recently and I just noticed that this is failing for the non-pr case:
It seems like the compatibility layer doesn't guarantee that this env var is not set to an empty string somewhere. woodpecker/pipeline/frontend/metadata/drone_compatibility.go Lines 69 to 71 in 383bfbb
this logic looks like it might potentially be problematic, because it checks a different variable for empty, but I think it is "fine" because the variable it checks was the source of this one. So it's probably ok? Somehow the plugin thinks this env var is empty though. I wonder if perhaps neither of these are set when building on the main branch, and that it is the plugin that is inferring an empty string? Here's the definition of the struct in that plugin: // PullRequest provides the pull request metadata.
PullRequest struct {
Number int `envconfig:"DRONE_PULL_REQUEST"`
} I'm not familiar enough with go to know how to reason about the behavior of this. |
This reverts commit 401df30. The docker merge step is failing: ``` envconfig.Process: assigning PULLREQUEST_DRONE_PULL_REQUEST to Number: converting '' to type int. details: strconv.ParseInt: parsing "": invalid syntax ``` It seems that the compatibility change in woodpecker-ci/woodpecker#3939 isn't sufficient. Let's see if we can still run with the environment set manually, or if we'll need to blackjack our own woodpecker plugin for this.
@wez this patch is in the main branch targeting the release of v3.0.0 ... so it's not released jet |
or do you men to do: if env["CI_COMMIT_PULL_REQUEST"] == "" {
env["PULLREQUEST_DRONE_PULL_REQUEST"] = "0"
+ env["DRONE_PULL_REQUEST"] = "0"
} ? |
It not being present in 2.7.1 definitely explains it! I think this might be clearer logic: - if env["CI_COMMIT_PULL_REQUEST"] == "" {
+ if env["PULLREQUEST_DRONE_PULL_REQUEST"] == "" {
env["PULLREQUEST_DRONE_PULL_REQUEST"] = "0"
} I reverted back to the deprecated environment setting syntax, and my pipeline succeeds, so I think I'm good to punt until 3.0.0 is released. The only consequence is that it is flagged with 39 warnings, so it may be difficult to notice future issues. I'll survive :) |
ah @wez you struggle with the fact that if you set environment, the plugin will not be considered as plugin anymore ... (security stuff) |
I'm not sure that I understand what you mean there. I have this in my config, on 2.7.1: merge-docker-image-arm:
depends_on: [build-docker-image-arm]
when:
- evaluate: 'platform == "linux/aarch64" && UBUNTU_IMAGE == "ubuntu:22.04" && CI_PIPELINE_EVENT == "tag"'
image: plugins/manifest
failure: ignore
environment: &docker_manifest_env
# Legacy env var to prevent the plugin from throwing an error
# when converting an empty string to a number
PULLREQUEST_DRONE_PULL_REQUEST: 0
settings: &docker_manifest_settings_tag
<<: *docker_credentials
target: "ghcr.io/kumocorp/kumomta-dev"
template: "ghcr.io/kumocorp/kumomta:${CI_COMMIT_TAG}-ARCH"
tags:
- latest
- ${CI_COMMIT_TAG}
platforms:
- linux/amd64
- linux/arm64 the presence of the I understand that the removal of the |
#3909 (comment)
cc @wez are you sure its
PULLREQUEST_DRONE_PULL_REQUEST
and notDRONE_PULL_REQUEST