fix: commit generated go-bindata files with "--no-compress" option by andyliuliming · Pull Request #1088 · Azure/aks-engine

andyliuliming · 2019-04-18T02:27:18Z

so do go-bindata here will help projects which vendored aks-engine.
and add no-compress option to go-bindata, so we will not need to rebase it everytime.

Reason for Change:

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Notes:

mboersma · 2019-04-18T15:10:57Z

/azp run pr-e2e

add no-compress option to go-bindata, so we will not need to rebase it everytime

👍 That's awesome if the fix is something simple like that we have overlooked.

azure-pipelines · 2019-04-18T15:11:30Z

For the Azure DevOps organization aks-engine, no matching pipelines using the Azure Pipelines app were found for this pull request.

mboersma · 2019-04-18T20:28:50Z

/azp run pr-e2e

azure-pipelines · 2019-04-18T20:29:09Z

Azure Pipelines successfully started running 1 pipeline(s).

jackfrancis · 2019-04-19T20:13:40Z

Just to be clear, the only de-optimization from this change will be the storage overhead of this file in the repo? ( And also to github's backend merge conflict validation service :| )

andyliuliming · 2019-04-20T01:44:52Z

Just to be clear, the only de-optimization from this change will be the storage overhead of this file in the repo? ( And also to github's backend merge conflict validation service :| )

yes, and the file size is not too big. so I think it's acceptable?

and for the future change, we can also verify if we change the files under "parts" folder, and the real change shows in the go files too,
before this change, the content is compressed, and not readable and we can only trust it :)

mboersma · 2019-04-23T13:30:54Z

/azp run pr-e2e

azure-pipelines · 2019-04-23T13:31:18Z

Azure Pipelines successfully started running 1 pipeline(s).

codecov · 2019-04-23T13:50:36Z

Codecov Report

Merging #1088 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1088   +/-   ##
=======================================
  Coverage   74.81%   74.81%           
=======================================
  Files         128      128           
  Lines       18318    18318           
=======================================
  Hits        13705    13705           
  Misses       3829     3829           
  Partials      784      784

jackfrancis · 2019-04-23T20:09:26Z

How often do we generate pkg/i18n/translations_generated.go? Should we add --no-compress to that as well?

andyliuliming · 2019-04-24T03:09:20Z

How often do we generate pkg/i18n/translations_generated.go? Should we add --no-compress to that as well?

do not know how often that file would be generated :)
this file is genearted by pkg/i18n/resourceloader.go
and '--no-compress' is added there too.

andyliuliming · 2019-04-25T00:19:59Z

Hi @jackfrancis @mboersma , could you please help merge this PR if no other comments? so I can do the according change in ACSRP repo. thanks.

jackfrancis · 2019-04-25T20:04:13Z

@mboersma to validate docs and E2E changes I just made

mboersma

This looks good to me, although we should be willing to revert if we run into real-world race conditions again.

.pipelines/pr-e2e.yaml

mboersma

/lgtm

CecileRobertMichon · 2019-04-25T20:46:26Z

/azp run pr-e2e

azure-pipelines · 2019-04-25T20:46:39Z

Azure Pipelines successfully started running 1 pipeline(s).

CecileRobertMichon · 2019-04-25T20:47:23Z

docs/community/developer-guide.md

 1. Once the PR is approved and the end-to-end job has passed, the PR can now be merged into the master branch
 1. Once merged, another job is triggered to verify integrity of the master branch. This job is similar to the PR job.
+
+## Pull Requests and Generated Code


mboersma · 2019-04-25T20:52:08Z

It's failing the ensure-generated check now. :-(

/lgtm cancel

jackfrancis · 2019-04-26T02:02:22Z

// TODO we need to add PR validation that confirms the hygiene of the source against the generated code within the fork/branch itself. Until then we can't move this forward.

andyliuliming · 2019-04-26T11:23:00Z

@jackfrancis @mboersma I took a look at the ensure-generated, I think that will protect us from the case that the generated go file not update to date. will generate the file again and pushing.

jackfrancis · 2019-04-26T16:12:21Z

/azp run pr-e2e

azure-pipelines · 2019-04-26T16:12:34Z

Azure Pipelines successfully started running 1 pipeline(s).

mboersma · 2019-04-26T19:29:22Z

@andyliuliming I'm still worried that this will create a situation that requires PRs to be rebased often in order to pass make ensure-generated.

Overall it seems that /parts is the most active area of change in the project, and any time something changes under that directory, the generated code needs to be updated. Once that's done, make ensure-generate will no longer pass for existing PRs: they will all need to be rebased against master.

This situation has happened before and it is a frustrating cycle that repels contributors. We absolutely don't want to do that—AKS Engine is better served by making it easy for people to contribute even if it means our Go packaging is slightly clumsy.

I'm trying to think of ways around this but don't have any fresh ideas.

andyliuliming · 2019-05-02T13:28:40Z

Hi @mboersma I suppose each PR-e2e build in VSTS will do
pull the master and apply the commits in a seperate branch (just like the rebasing) and then run the tests, ours pipeline did not do that?

CecileRobertMichon · 2019-05-02T16:58:00Z

Even if the pipeline does merge master before running the tests, developers will still run into merge conflicts (thus requiring a rebase) every time they change one of the files that requires an update to the generated code (eg. parts/). Running the pipeline tests on a rebased branch will only help make ensure-generated pass when no file that requires files to be generated was changed.

andyliuliming · 2019-05-03T03:20:30Z

Even if the pipeline does merge master before running the tests, developers will still run into merge conflicts (thus requiring a rebase) every time they change one of the files that requires an update to the generated code (eg. parts/). Running the pipeline tests on a rebased branch will only help make ensure-generated pass when no file that requires files to be generated was changed.

"developers will still run into merge conflicts (thus requiring a rebase) every time they change one of the files that requires an update to the generated code (eg. parts/)"

I think the git will do the auto-merge for us if there's no real code conflict after this code change.
we can do a test.

andyliuliming · 2019-05-07T13:23:59Z

@mboersma @CecileRobertMichon any updates?
do you mind grant me the permission to trigger the pipeline, so I can do a test if the "merge conflicts" will happen(probably it won't happen) on a separate branch.

I think the reason the git could not do the auto-merge just because we do not set the no-compress option.

and after this change, say if someone changed the file in the parts file.

then the diff for the go code generated would be like:

+ code change one
- code change two

and the git will do the auto-merging for us.

CecileRobertMichon · 2019-05-07T16:46:41Z

@andyliuliming I've granted you write access you should now be able to trigger the pipeline using /azp run pr-e2e. (you don't need to trigger the pipeline to see if there are merge conflicts though, opening a PR is enough).

so do go-bindata here will help projects which vendored aks-engine. and add no-compress option to go-bindata, so we will not need to rebase it everytime.

mboersma · 2019-05-09T13:50:34Z

I do think git will merge changes in the generated files correctly in most cases. The bottleneck will be the make ensure-generated task.

Azure DevOps merges the branch into master before running tests, so any changes in master that haven't been incorporated into your branch will cause make ensure-generated to fail, since it will regenerate the files and there will be a diff. Then you need to rebase against master and force-push—and hope that no other changes to parts/ have been committed to master in the meantime.

(I may be wrong: this chicken-egg situation is confusing.)

CecileRobertMichon · 2019-05-10T18:43:40Z

#1255 seems to suggest that this is working

mboersma · 2019-05-10T19:37:37Z

#1255 seems to suggest that this is working

Yes...it does. I am happy to be wrong if so.

In that case I'm LGTM on this so long as we revert if it does somehow cause hassle for developers.

CecileRobertMichon · 2019-05-10T19:38:12Z

In that case I'm LGTM on this so long as we revert if it does somehow cause hassle for developers.

+ 1

jackfrancis · 2019-05-13T18:25:09Z

/lgtm

we'll monitor this in the coming days to ensure it doesn't increase rate of master merge conflicts

acs-bot · 2019-05-13T18:26:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyliuliming, jackfrancis

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:

~~OWNERS~~ [jackfrancis]

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

acs-bot added the size/XXL label Apr 18, 2019

andyliuliming mentioned this pull request Apr 18, 2019

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

Closed

4 tasks

mboersma added the approved label Apr 18, 2019

mboersma mentioned this pull request Apr 22, 2019

style: remove unneeded nil check #1110

Merged

4 tasks

andyliuliming force-pushed the andliu/go_bin_data branch from 436f1fb to 1d03810 Compare April 23, 2019 13:29

mboersma added the awaiting review label Apr 23, 2019

mboersma mentioned this pull request Apr 25, 2019

fix: clean generated and unit test helper files #1151

Merged

4 tasks

mboersma reviewed Apr 25, 2019

View reviewed changes

.pipelines/pr-e2e.yaml Outdated Show resolved Hide resolved

mboersma approved these changes Apr 25, 2019

View reviewed changes

acs-bot assigned mboersma Apr 25, 2019

acs-bot added the lgtm label Apr 25, 2019

mboersma removed the awaiting review label Apr 25, 2019

mboersma changed the title ~~fix: go mod would not copy the non-go files by default~~ fix: commit generated go-bindata files with "--no-compress" option Apr 25, 2019

CecileRobertMichon reviewed Apr 25, 2019

View reviewed changes

andyliuliming force-pushed the andliu/go_bin_data branch from 4b0210c to 5a1dd7e Compare April 26, 2019 11:25

andyliuliming and others added 6 commits May 8, 2019 10:54

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

d3c6712

so do go-bindata here will help projects which vendored aks-engine. and add no-compress option to go-bindata, so we will not need to rebase it everytime.

chore: add make ensure-generated to CI, and docs

25e850e

chore: \t removal

e41f125

fix: regenerate the go file so make it up-to-date.

6cfdbe9

fix: the *_generated.go is checked in as source code now.

2645d3b

fix: re-regerate it so the files up-to-date.

a50327c

andyliuliming force-pushed the andliu/go_bin_data branch from d9c249b to a50327c Compare May 8, 2019 02:56

andyliuliming mentioned this pull request May 8, 2019

[FakeFake] fix: this is a fake change. #1255

Closed

4 tasks

acs-bot assigned jackfrancis May 13, 2019

acs-bot added the lgtm label May 13, 2019

acs-bot merged commit 39c2a4f into Azure:master May 13, 2019

This was referenced May 14, 2019

chore: update templates_generated.go #1295

Merged

generated code files are falling out of sync #1318

Closed

Conversation

andyliuliming commented Apr 18, 2019

Uh oh!

mboersma commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azure-pipelines bot commented Apr 18, 2019

Uh oh!

mboersma commented Apr 18, 2019

Uh oh!

azure-pipelines bot commented Apr 18, 2019

Uh oh!

jackfrancis commented Apr 19, 2019

Uh oh!

andyliuliming commented Apr 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mboersma commented Apr 23, 2019

Uh oh!

azure-pipelines bot commented Apr 23, 2019

Uh oh!

codecov bot commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jackfrancis commented Apr 23, 2019

Uh oh!

andyliuliming commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andyliuliming commented Apr 25, 2019

Uh oh!

jackfrancis commented Apr 25, 2019

Uh oh!

mboersma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mboersma left a comment

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon commented Apr 25, 2019

Uh oh!

azure-pipelines bot commented Apr 25, 2019

Uh oh!

CecileRobertMichon Apr 25, 2019

Choose a reason for hiding this comment

Uh oh!

mboersma commented Apr 25, 2019

Uh oh!

jackfrancis commented Apr 26, 2019

Uh oh!

andyliuliming commented Apr 26, 2019

Uh oh!

jackfrancis commented Apr 26, 2019

Uh oh!

azure-pipelines bot commented Apr 26, 2019

Uh oh!

mboersma commented Apr 26, 2019

Uh oh!

andyliuliming commented May 2, 2019

Uh oh!

CecileRobertMichon commented May 2, 2019

Uh oh!

andyliuliming commented May 3, 2019

Uh oh!

andyliuliming commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CecileRobertMichon commented May 7, 2019

Uh oh!

mboersma commented May 9, 2019

Uh oh!

CecileRobertMichon commented May 10, 2019

Uh oh!

mboersma commented May 10, 2019

Uh oh!

CecileRobertMichon commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mboersma commented Apr 18, 2019 •

edited

Loading

andyliuliming commented Apr 20, 2019 •

edited

Loading

codecov bot commented Apr 23, 2019 •

edited

Loading

andyliuliming commented Apr 24, 2019 •

edited

Loading

andyliuliming commented May 7, 2019 •

edited

Loading

CecileRobertMichon commented May 10, 2019 •

edited

Loading