Skip to content

fix: always use LocalOnlyGitRepo source with dry_run command#1452

Merged
manicmaniac merged 1 commit into
danger:masterfrom
imaginaris:fix/dry_run-on-ci
Feb 4, 2024
Merged

fix: always use LocalOnlyGitRepo source with dry_run command#1452
manicmaniac merged 1 commit into
danger:masterfrom
imaginaris:fix/dry_run-on-ci

Conversation

@imaginaris

Copy link
Copy Markdown
Contributor

This PR should fix an issue that happens in my project.

We are running danger dry_run on master branch (no PR) on Bitrise CI machines.
After upgrading ruby version from 2.7.6 to 3.1.4 we started to get following error:
/Users/vagrant/.asdf/installs/ruby/3.1.4/lib/ruby/gems/3.1.0/gems/danger-9.3.1/lib/danger/request_sources/bitbucket_cloud_api.rb:125:in fetch_json': Credentials not available. Provide DANGER_BITBUCKETCLOUD_USERNAME, DANGER_BITBUCKETCLOUD_UUID, and DANGER_BITBUCKETCLOUD_PASSWORD as environment variables. (RuntimeError)`

Even though we use Github.com repos and the same Danger version (9.3.1) as before.

The CI.available_ci_sources is a set and local_ci_source method in environment_manager.rb uses a find function to search for the first valid source in that set. A set has undefined order of elements when treated as a list so when there are multiple CI sources valid (in my case Bitrise and LocalOnly), the local_ci_source method (theoretically) can return a different value each time.
Returning Danger::Bitrise value in my case can't work because there is no PR info in the system env when I'm running danger dry_run on no-PR build. (Bitrise class tries to init all RequestSource classes from supported_request_sources one by one).

Long story short, when running danger dry_run only LocalOnlyGitRepo should be used as a CI source.
I did my best in this PR (I'm not a ruby dev 🙂) so any comments are welcome.

@manicmaniac

Copy link
Copy Markdown
Member

@imaginaris
Thanks you for the detailed description. The change looks reasonable to me.
Could you rebase it on master and resolve conflicts?

@manicmaniac manicmaniac self-requested a review February 3, 2024 03:32
@imaginaris

Copy link
Copy Markdown
Contributor Author

@manicmaniac Done.

@manicmaniac

Copy link
Copy Markdown
Member

@imaginaris Could you resolve the following lint error?

https://github.com/danger/danger/actions/runs/7768462603/job/21188021833?pr=1452

Run bundle exec rubocop
Inspecting 219 files
.........................................................................C.................................................................................................................................................

Offenses:

lib/danger/danger_core/environment_manager.rb:10:[7](https://github.com/danger/danger/actions/runs/7768462603/job/21188021833?pr=1452#step:5:8): C: [Correctable] Layout/EmptyLineAfterGuardClause: Add empty line after guard clause.
      return Danger::LocalOnlyGitRepo if LocalOnlyGitRepo.validates_as_ci? env
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

21[9](https://github.com/danger/danger/actions/runs/7768462603/job/21188021833?pr=1452#step:5:10) files inspected, 1 offense detected, 1 offense autocorrectable

@imaginaris

Copy link
Copy Markdown
Contributor Author

@manicmaniac Fixed. The CI job should pass now.

@manicmaniac manicmaniac merged commit 105e2de into danger:master Feb 4, 2024
@imaginaris imaginaris deleted the fix/dry_run-on-ci branch February 5, 2024 21:17

# Finds a Danger::CI class based on the ENV
def self.local_ci_source(env)
return Danger::LocalOnlyGitRepo if LocalOnlyGitRepo.validates_as_ci? env

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

regression: There should be a way to not fall into this if. I lost the capability to simulate my workflows locally that depend on the gitlab API, and the same will happen for other SCMs =/

In my scenario, I run bundle exec danger staging --pry with some gitlab envs such GITLAB_CI, etc.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Upon reviewing the code, this MR makes much more sense than relying on CI load ordering.

My workaround for local simulation is to wrap the pry as it happens in the staging command, and run it as simple danger execution with the CI variables correctly set. Sorry for the confusion 😅

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.

3 participants