Skip to content

remove lintconfig.json, that's generated#1869

Merged
istio-merge-robot merged 6 commits intoistio:masterfrom
jmuk:lintconfig
Nov 28, 2017
Merged

remove lintconfig.json, that's generated#1869
istio-merge-robot merged 6 commits intoistio:masterfrom
jmuk:lintconfig

Conversation

@jmuk
Copy link
Copy Markdown
Contributor

@jmuk jmuk commented Nov 27, 2017

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

none

@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Nov 27, 2017

/assign @guptasu

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 27, 2017

Codecov Report

Merging #1869 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1869      +/-   ##
==========================================
- Coverage    81.9%   81.89%   -0.01%     
==========================================
  Files         188      189       +1     
  Lines       18715    18721       +6     
==========================================
+ Hits        15328    15332       +4     
- Misses       2960     2961       +1     
- Partials      427      428       +1
Flag Coverage Δ
#broker 45.51% <ø> (+1.06%) ⬆️
#mixer 82.49% <ø> (ø) ⬆️
#pilot 82.23% <ø> (-0.05%) ⬇️
#security 90.39% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pilot/adapter/config/crd/controller.go 77.85% <0%> (-2.02%) ⬇️
broker/pkg/version/version.go 100% <0%> (ø)
mixer/adapter/stackdriver/metric/bufferedClient.go 92.5% <0%> (+2.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc39692...d76aff4. Read the comment docs.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 27, 2017

Codecov Report

Merging #1869 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1869      +/-   ##
==========================================
- Coverage   81.23%   81.18%   -0.06%     
==========================================
  Files         191      187       -4     
  Lines       19344    18778     -566     
==========================================
- Hits        15715    15245     -470     
+ Misses       3173     3106      -67     
+ Partials      456      427      -29
Flag Coverage Δ
#broker 45.51% <ø> (ø) ⬆️
#mixer 82.51% <ø> (-0.03%) ⬇️
#pilot 80.31% <ø> (-0.17%) ⬇️
#security 90.39% <ø> (ø) ⬆️
Impacted Files Coverage Δ
broker/pkg/model/config/mock_store.go 18.57% <ø> (ø) ⬆️
pilot/platform/kube/queue.go 82.69% <0%> (-7.7%) ⬇️
mixer/adapter/prometheus/server.go 89.83% <0%> (-5.09%) ⬇️
pilot/platform/kube/inject/inject.go
pilot/platform/kube/inject/http.go
pilot/platform/kube/inject/initializer.go
pilot/platform/kube/inject/configmap.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85983c2...40ab2a7. Read the comment docs.

generated_files Outdated
mixer/test/spyAdapter/template/report/go_default_library_handler.gen.go
mixer/test/spyAdapter/template/report/go_default_library_tmpl.pb.go
mixer/test/spyAdapter/template/template.gen.go
mixer/test/template/report/go_default_library_handler.gen.go
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bottom 3 files must be gone now. I think

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jmuk how is this file updates ? Any idea why this change was missed when @geeknoid submitted the PR that moved the .gen.go files ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks.

Those files are updated through bazel_to_go.py script, but that change doesn't make presubmit fail, it's very easy to miss that. Currently I'm seeing some difficulty since some presubmit run can affect others through the build caches (see #1689 and #1697 FYI).

.gitignore Outdated
# pilot
pilot/platform/config
# lint
lintconfig.json
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

instead, should we just generated the temporary file in some /tmp dir ? replacing it every time the lint.sh script runs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

conf = json.load(fin)
conf['exclude'].extend(manifest)
with open(os.path.join(WKSPC, "lintconfig.json"), "wt") as fout:
with open(os.path.join("/tmp", "istio-lintconfig.json"), "wt") as fout:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this might not work on mac. I usually have seen on Mac the temp did is named /T and not /tmp

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If there is no easy way to put the file in tmp dir, we can just leave it to what you had before... adding the file to the .ignore file. May be renamed it to istio-lintconfig.gen.json

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well then, that's a bit hard; tempfile.gettempdir() can be used here, but the same path needs to be specified in linters.sh script, I'm not sure how it can be specified in a platform agnostic way; let me revert back for that part, and use lintconfig.gen.json name (I don't think istio- prefix is needed in that case, obviously it's for istio when it's within the workspace).

@guptasu
Copy link
Copy Markdown
Contributor

guptasu commented Nov 27, 2017

/lgtm

@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Nov 27, 2017

/assign @ldemailly

@ldemailly
Copy link
Copy Markdown
Member

/lgtm

@istio-merge-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guptasu, ldemailly

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

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot istio-merge-robot merged commit 5741551 into istio:master Nov 28, 2017
vadimeisenbergibm pushed a commit to vadimeisenbergibm/istio that referenced this pull request Nov 28, 2017
Automatic merge from submit-queue.

remove lintconfig.json, that's generated

**What this PR does / why we need it**:

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
none
```
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.

6 participants