-
Notifications
You must be signed in to change notification settings - Fork 919
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
feat(terraform): defined nearly all supported Terraform platforms for locking #4784
feat(terraform): defined nearly all supported Terraform platforms for locking #4784
Conversation
5c24eba
to
0a4d2ba
Compare
Thanks for looking into this! One possible concern I have is that for folks that don't run terraform with these options locally, this would result in large diffs (and possibly merge conflicts because of those) with changes made locally vs by Dependabot Curious to get your thoughts on this |
6f0d528
to
de3f846
Compare
Hey @jurre I wouldn't expect merge conflicts, but for sure larger diffs initially. |
FWIW, the current situation is likely far worse, in terms of conflicts with changes made locally Currently every Windows + Mac user has dependabot continuously removing the right architectures, resulting in a lot of people always having uncommitted changes from running So every time there's a change and they pull, they have to stash/discard those changes With this, people will no longer need to routinely change their local |
correct @billinghamj 😄 |
@jurre anything still open to be clarified? |
Hi @sebbrandt87 and @billinghamj, This is something that's been on our radar for a bit. I agree that Dependabot continuously removing the right architectures is pretty frustrating, so I'm open to merging this change in for the time being. I ran some numbers on the time it took for Terraform to download hashes for mac, linux, and windows (x64) versus just one platform, and it looks like it takes ~60% longer, which makes sense, but isn't ideal. It also results in more outbound calls (where it could error) and bloats the lockfile, like mentioned before. We discussed a couple solutions:
version: 2
updates:
- package-ecosystem: 'terraform'
platform: 'linux'
directory: '/'
schedule:
interval: 'daily'
What do y'all think? |
Hey @Nishnha |
I'd be very, very happy with either of those options :) (though agree #1 is best, much clearer) That said, in the temporary absence of either of those - if they wouldn't be realistic in the near term, I'd very much appreciate having this more "brute force" approach in the meantime |
… locking For the purpose of Terraform locking, we now have defined nearly all supported Terraform platforms in the `terraform providers lock` command. Except linux_arm64, as not all providers seem to be present yet for that platform. This makes sure that we have all h1 sums in the `.terraform.lock.hcl` so that we keep compatability and do not run in errors on runs at other platforms than Linux. resolves: dependabot#4042
de3f846
to
5f0d559
Compare
Sounds like consensus. I'll work with the Dependabot team to track this work and draft a release plan! Hope we can resolve this pain point soon! |
Could we maybe talk to the terraform folks and see if we can get something like a |
@jurre I think that is very unlikely to be considered. Terraform is developed quite slowly, and having now reached 1.0, is no longer changing significantly in how it works. There is also already good support for this use-case, and it automatically chooses the right options for the architecture you're using. Adding a feature just for a team inside GitHub seems unlikely |
Surely users run into this running terraform locally as well, right? 😕 If you run on |
I bet we can also create a PR to Terraform, so that we maybe get something like a . But as dependabot does atomic updates to the providers, without an option for this convention to be set and respected, we still have this issue. Can't we proceed as @Nishnha said a few days ago for now, and we come back to remove this "hard" locking in the next iterations of releases / features being implemented? Please ❤️ 😊 EDIT Friendly ping @jurre |
@@ -14,6 +14,7 @@ class FileUpdater < Dependabot::FileUpdaters::Base | |||
PRIVATE_MODULE_ERROR = /Could not download module.*code from\n.*\"(?<repo>\S+)\":/.freeze | |||
MODULE_NOT_INSTALLED_ERROR = /Module not installed.*module\s*\"(?<mod>\S+)\"/m.freeze | |||
GIT_HTTPS_PREFIX = %r{^git::https://}.freeze | |||
TF_PLATFORMS = "-platform=linux_amd64 -platform=darwin_amd64 -platform=windows_amd64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make sure to include Apple Silicon support too?
TF_PLATFORMS = "-platform=linux_amd64 -platform=darwin_amd64 -platform=windows_amd64" | |
TF_PLATFORMS = "-platform=linux_amd64 -platform=darwin_amd64 -platform=darwin_arm64 -platform=windows_amd64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure if we can handle this atm, we would need to make sure that for all basic terraform providers we have the darwin_arm64
build available. I can tell that we don't have the linux_arm64
nor darwin_arm64
available for all, that is why I excluded it for now from the static TF_PLATFORMS. To check some you can have a look at https://releases.hashicorp.com
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm this probably needs to be configurable rather than hardcoded then I think - unless e.g. it can just add all available platforms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this reason we talked about #4784 (comment).
But for now we should have at least something to drop pain points most of us share on a daily / weekly basis 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The number of people using Apple Silicon MacBooks now is pretty huge - so I don't think this prevents the pain for "most" at this point really
I think I'll look into making a quick workflow which just updates the dependabot PRs to add on the other providers
For the benefit of anyone remaining frustrated at this problem still not getting any traction, you can make an actions workflow to deal with this short-term: https://gist.github.com/billinghamj/2a127b9cbd475e6b210d10431ccdde30 @sebbrandt87 in case you find it helpful To GitHub - this still needs to be fixed. It's very disappointing that you agreed to work with us to figure this out, that you said you'd release this PR, and that a month later you have gone silent. As time goes on, it gets increasingly more difficult to recommend Dependabot to other engineers. |
Hi all, sorry for my general lack of response on this issue. I should have posted this update sooner. I wanted to try out automatically detecting platform architectures by checking against the existing hashes in I realized, however, that if we merge in this PR, we would have already written other architectures to a user's lockfile, and we would need to reverse that if we try to automatically detect architectures. The only way I can think of reversing this is if we force users manually specify them again with Given the "just work" nature that we are going for with Dependabot, we will need to think of a way to reverse this PR or we may unintentionally break Terraform projects for many users. Please let me know if you all think of a solution so we can get this PR merged in. The code for automatically detecting Terraform architectures (#4905) is still failing one of its tests and hasn't been fully optimized, but the Dependabot team would like to move forward with that approach if possible. We also would like to eventually work with Hashicorp to bring a Sorry for changing things from what I had originally stated and thank you for the Action @billinghamj. |
Thank you @Nishnha - it's great to understand how the work has changed, and makes sense that this could conflict with it. Will follow the other PR longer term :) |
Opened #4905 for review |
Closing in favor of #4905 |
For the purpose of Terraform locking, we now have defined nearly all supported Terraform platforms in the
terraform providers lock
command.Except linux_arm64, as not all providers seem to be present yet for that platform.
This makes sure that we have all h1 sums in the
.terraform.lock.hcl
so that we keep compatability and do not run in errors on runs at other platforms than Linux.Quote Terraform docs
resolves: #4042