Skip to content
This repository was archived by the owner on Oct 24, 2023. It is now read-only.

fix: go mod would not copy the non-go files by default#1078

Closed
andyliuliming wants to merge 1 commit intoAzure:masterfrom
andyliuliming:andliu/go_bin_data
Closed

fix: go mod would not copy the non-go files by default#1078
andyliuliming wants to merge 1 commit intoAzure:masterfrom
andyliuliming:andliu/go_bin_data

Conversation

@andyliuliming
Copy link
Member

Reason for Change:

fix: go mod would not copy the non-go files by default, so do go-bindata here is better. which will help the projects which vendored aks-engine .

Issue Fixed:

Requirements:

Notes:

@acs-bot
Copy link

acs-bot commented Apr 17, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: andyliuliming
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: serbrech

If they are not already assigned, you can assign the PR to them by writing /assign @serbrech in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mboersma
Copy link
Member

@andyliuliming can you make sure your commit messages follow https://www.conventionalcommits.org/? Otherwise they don't show up in the changelog we generate.

so do go-bindata here will help projects which vendored aks-engine.
@andyliuliming andyliuliming changed the title go mod would not copy the non-go files by default fix: go mod would not copy the non-go files by default Apr 17, 2019
@andyliuliming
Copy link
Member Author

@andyliuliming can you make sure your commit messages follow https://www.conventionalcommits.org/? Otherwise they don't show up in the changelog we generate.

done, thanks.

@mboersma
Copy link
Member

We have tried before to check the go-bindata-generated files into version control, but found it created a git rebase nightmare. We reverted that change in #546. I don't think we want to do this again.

The tradeoff is that you have to run make build or make generate after go get github.com/Azure/aks-engine, but that's the compromise we chose.

@jackfrancis
Copy link
Member

What @mboersma said is correct. We can't accept this change as-is.

@andyliuliming
Copy link
Member Author

andyliuliming commented Apr 17, 2019

but if we switch to use the go module.
the go mod vendor will not copy the parts files for us.
so the go generate will fail at the repos which try to vendor aks-engine :(
@jackfrancis @mboersma

@mboersma
Copy link
Member

So #1077 won't work without this change?

Theoretically, the binary data could be broken into small segments to ensure there was minimal contention over changes. If there were a very close mapping between the script or code and its go-bindata representation, then real-world merge conflicts on PRs would be meaningful.

But I doubt we would want to make things that complicated—if that approach would even work. In practice, checking in these files required every PR in the queue to be rebased every time one was merged.

@mboersma mboersma mentioned this pull request Apr 17, 2019
4 tasks
@andyliuliming
Copy link
Member Author

So #1077 won't work without this change?

Theoretically, the binary data could be broken into small segments to ensure there was minimal contention over changes. If there were a very close mapping between the script or code and its go-bindata representation, then real-world merge conflicts on PRs would be meaningful.

But I doubt we would want to make things that complicated—if that approach would even work. In practice, checking in these files required every PR in the queue to be rebased every time one was merged.

then one no-compress option will help :)

re-send one PR: #1088

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants