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.

Sign up for GitHub

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

Jump to bottom

updates to mm after removing vendor directories downstream #3436

Merged
merged 15 commits into from
May 13, 2020
Merged

updates to mm after removing vendor directories downstream #3436

merged 15 commits into from
May 13, 2020

Conversation

megan07
Copy link
Contributor

@megan07 megan07 commented Apr 28, 2020 •
edited
Loading

Addresses hashicorp/terraform-provider-google#5789

updates to mm after removing vendor directories downstream

there are other references to vendor in here, but wasn't sure if i should be removing all of them as i'm not sure how it will affect other downstreams.

will merge this at the same time as:
TPG: hashicorp/terraform-provider-google#6219
TPGB: hashicorp/terraform-provider-google-beta#2003

Release Note Template for Downstream PRs (will be copied)

@megan07 megan07 requested a review from nat-henderson April 28, 2020 17:10
nat-henderson
nat-henderson reviewed Apr 28, 2020
View reviewed changes
.ci/containers/downstream-builder/Dockerfile
@@ -9,8 +9,7 @@ RUN go get github.com/github/hub
RUN ssh-keyscan github.com >> /known_hosts
RUN echo "UserKnownHostsFile /known_hosts" >> /etc/ssh/ssh_config

ENV GOFLAGS "-mod=vendor"
ENV GO111MODULE "off"
ENV GO111MODULE "on"
Copy link
Contributor

Choose a reason for hiding this comment

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

Historically this has caused goimports to hang indefinitely and we don't know why. It might be the vendor directory, but it also might not be! We'll have to test it to find out. Do you have a branch of terraform that's already had the vendor directory removed so we can check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! They're megan_rm_vendoring in both downstreams as well.

@megan07 megan07 force-pushed the megan_rm_vendoring branch from 29bf280 to d060bd8 Compare May 7, 2020 15:10
@megan07
Copy link
Contributor Author

megan07 commented May 7, 2020 •
edited
Loading

For some context - it was found that goimports behaved differently if running on a file in the TPG directory from magic-modules compared to running it from the TPG directory, so to solve this we change to the TPG directory to make these calls. I'm not sure how these changes may affect generating code in the other repositories.

nat-henderson
nat-henderson approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@nat-henderson nat-henderson left a comment

Choose a reason for hiding this comment

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

I'm comfortable testing this in the magician when we submit it, sure. :) Recommend you wait for the other reviewers, and please let's make sure I (or someone who knows the magician) am free when it goes in - I'll want to watch carefully.

compile/core.rb Outdated
if !$pwd
$pwd = Dir.pwd
end
compile_string(ctx, File.read($pwd+"/"+source))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really change things? That's so wild.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Want to document our conversation from yesterday, but this changed because we're now cd-ing into the TPG directory and calling from there, so $pwd is the magic-module directory and this now calls the absolute path.

@nat-henderson
Copy link
Contributor

Oh, you do have to change some tests, but the test failures don't represent real issues.

@megan07 megan07 force-pushed the megan_rm_vendoring branch from 0529e6f to da123f2 Compare May 12, 2020 19:14
@nat-henderson
Copy link
Contributor

Is this ready to go in? I am willing and able to disable the "ci evil filter" so that we can test what will be generated downstream! :)

@megan07
Copy link
Contributor Author

megan07 commented May 12, 2020

It's ready from my end now, I think. I'm not sure why the build is failing. And I'm not sure how it's doing for Inspec or Ansible? But the lint errors are cleaned up and it looks like what's generated downstream for Terraform (locally) is working correctly!

@slevenick
Copy link
Contributor

It's ready from my end now, I think. I'm not sure why the build is failing. And I'm not sure how it's doing for Inspec or Ansible? But the lint errors are cleaned up and it looks like what's generated downstream for Terraform (locally) is working correctly!

I think the best way to test it is to have MM generate the downstreams and see if there are any diffs

@nat-henderson
Copy link
Contributor

Okay, I have deployed a thing that should grant this specific PR an exception to the "no potentially dangerous PRs can run in automation" rule. :) I'll go hit "retry" and we'll see what happens.

@nat-henderson
Copy link
Contributor

Ah, of course - actually we'll first have to build and deploy the container that's been modified. We'll have to shut down merging into this repo for a while to be safe.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 7 insertions(+), 6 deletions(-))
Terraform Beta: Diff ( 3 files changed, 5 insertions(+), 5 deletions(-))

@megan07 megan07 merged commit 4807251 into GoogleCloudPlatform:master May 13, 2020
@modular-magician modular-magician mentioned this pull request May 13, 2020
Merged
nathkn pushed a commit to nathkn/magic-modules that referenced this pull request May 18, 2020
@megan07
updates to mm after removing vendor directories downstream (GoogleClo…
4b9493f 
…udPlatform#3436)

* updates to mm after removing vendor directories downstream

* more changes to magic-modules for removing vendoring

* fix tests

* lint fixes

* fix more lint errors

* fix more lint errors

* fix more lint errors

* fix the rest of the lint errors

* update generate in ansbile.rb

* update  ansbile.rb

* inspec changes (and some minor terraform stuff)

* updates for ansible

* updates for tf oics and conversion

* made changes for ansible_devel

* updates for linter
@daniel-cit daniel-cit mentioned this pull request Nov 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nat-henderson nat-henderson nat-henderson approved these changes

@rambleraptor rambleraptor Awaiting requested review from rambleraptor

@slevenick slevenick Awaiting requested review from slevenick

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@megan07 @nat-henderson @slevenick @modular-magician @googlebot