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

Fix common CI #8347

Merged
merged 2 commits into from
Nov 7, 2023
Merged

Fix common CI #8347

merged 2 commits into from
Nov 7, 2023

Conversation

deivid-rodriguez
Copy link
Contributor

Apparently #8310 broke some common specs. No idea why CI did not catch that, but can be reproduced with

rspec ./spec/dependabot/file_fetchers/base_spec.rb:1646

from common.

This PR removes the signature from run_shell_command until we can address the real culprit, since that seems to let the specs pass.

@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review November 6, 2023 18:39
@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner November 6, 2023 18:39
@deivid-rodriguez
Copy link
Contributor Author

@JamieMagee I think this may be a fun one for you 😃

@honeyankit
Copy link
Contributor

This PR removes the signature from run_shell_command

Just checking, Is there any harm in removing the signature

@deivid-rodriguez
Copy link
Contributor Author

I think @JamieMagee may answer that better but my understanding is that it just means less strict typing. It's definitely not a permanent solution, just a way to get our CI green for now and unblock other PRs. We would readd the signature and reenable strict typing as soon as we find the culprit.

@JamieMagee
Copy link
Contributor

JamieMagee commented Nov 6, 2023

Can you share an example of the failure?

@deivid-rodriguez is right, it's just less strict type-checking

EDIT: this is what I get locally

1) Dependabot::FileFetchers::Base with submodules #clone_repo_contents does not clone submodules by default
     Failure/Error:
       expect(Dependabot::SharedHelpers)
         .to have_received(:run_shell_command).with(
           /\Agit clone .* --no-recurse-submodules/
         )
     
       Dependabot::SharedHelpers received :run_shell_command with unexpected arguments
         expected: (/\Agit clone .* --no-recurse-submodules/)
              got: ("git config --global credential.helper '!/home/jamie/src/work/dependabot-core/common/lib/dependabot/../../bin/git-credential-store-immutable --file /home/jamie/src/work/dependabot-core/common/git.store'", {:allow_unsafe_shell_command=>true, :fingerprint=>"git config --global credential.helper '<helper_command>'"})
       Diff:
       @@ -1,3 +1,5 @@
       -[/\Agit clone .* --no-recurse-submodules/]
       +["git config --global credential.helper '!/home/jamie/src/work/dependabot-core/common/lib/dependabot/../../bin/git-credential-store-immutable --file /home/jamie/src/work/dependabot-core/common/git.store'",
       + {:allow_unsafe_shell_command=>true,
       +  :fingerprint=>"git config --global credential.helper '<helper_command>'"}]
       
     # ./spec/dependabot/file_fetchers/base_spec.rb:1649:in `block (4 levels) in <top (required)>'
     # ./spec/spec_helper.rb:47:in `block (2 levels) in <top (required)>'

@@ -1,6 +1,7 @@
--dir=.
--ignore=tmp/
--ignore=vendor/
--ignore=.bundle/
Copy link
Contributor

Choose a reason for hiding this comment

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

What makes this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, nothing, this is just because I run sorbet locally, and since I had files at bundler/helpers/v1/.bundle and bundler/helpers/v2/.bundle (probably because of having run bundler specs), sorbet showed a bunch of failures in those installed gems.

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez
Copy link
Contributor Author

I pushed a different change that allows specs to pass while keeping the strict typing. I guess this is just poor RSpec support.

@JamieMagee
Copy link
Contributor

I pushed a different change that allows specs to pass while keeping the strict typing. I guess this is just poor RSpec support.

Poor RSpec support in Sorbet?

@deivid-rodriguez
Copy link
Contributor Author

Yes, that's what I meant.

Just realized my change is not working, not sure why I thought it was 😢

@deivid-rodriguez
Copy link
Contributor Author

Ok, I pushed yet another different approach. This one should be fine I believe.

@deivid-rodriguez
Copy link
Contributor Author

I guess these could be related? In any case, I think it's best to not rely on these mocks and move on.

They don't seem to be playing nice with sorbet and run into failures
like:

```
Failures:

  1) Dependabot::FileFetchers::Base with submodules #clone_repo_contents does not clone submodules by default
     Failure/Error:
       expect(Dependabot::SharedHelpers)
         .to have_received(:run_shell_command).with(
           /\Agit clone .* --no-recurse-submodules/
         )

       Dependabot::SharedHelpers received :run_shell_command with unexpected arguments
         expected: (/\Agit clone .* --no-recurse-submodules/)
              got: ("git config --global credential.helper '!/home/dependabot/common/lib/dependabot/../../bin/git-credential-store-immutable --file /home/dependabot/common/git.store'", {:allow_unsafe_shell_command=>true, :fingerprint=>"git config --global credential.helper '<helper_command>'"})
       Diff:
       @@ -1,3 +1,5 @@
       -[/\Agit clone .* --no-recurse-submodules/]
       +["git config --global credential.helper '!/home/dependabot/common/lib/dependabot/../../bin/git-credential-store-immutable --file /home/dependabot/common/git.store'",
       + {:allow_unsafe_shell_command=>true,
       +  :fingerprint=>"git config --global credential.helper '<helper_command>'"}]

     # ./spec/dependabot/file_fetchers/base_spec.rb:1649:in `block (4 levels) in <top (required)>'
     # ./spec/spec_helper.rb:47:in `block (2 levels) in <top (required)>'
```

Let's do expectations on the resulting files after running the commands.
@deivid-rodriguez deivid-rodriguez merged commit 4d22252 into main Nov 7, 2023
80 checks passed
@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/fix-common-ci branch November 7, 2023 15:54
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.

4 participants