You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/01/06 20:53:12 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request #4260: Codecov
ocket8888 opened a new pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260
## What does this PR (Pull Request) do?
Adds code-coverage output to Go unit tests, also outputs junit XML
- [x] This PR is not related to any Issue
## Which Traffic Control components are affected by this PR?
None
## What is the best way to verify this PR?
Build and run the unit test service from `tools/golang/docker-compose.yml`, make sure it outputs what you want.
## The following criteria are ALL met by this PR
- [x] This PR includes tests OR I have explained why tests are unnecessary
- [x] This PR includes documentation OR I have explained why documentation is unnecessary
- [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
- [x] This PR includes any and all required license headers
- [x] This PR ensures that database migration sequence is correct OR this PR does not include a database migration
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] lbathina commented on issue #4260: Codecov
Posted by GitBox <gi...@apache.org>.
lbathina commented on issue #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#issuecomment-572159985
looks good to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] mhoppa commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r363513265
##########
File path: tools/golang/Dockerfile
##########
@@ -42,4 +42,10 @@ CMD ["golangci-lint", "run", "./..."]
FROM base AS unit
-CMD sh -c 'go test $(go list ./... | grep -v -e experimental -e "test/router" -e "api/v14")'
+VOLUME ["/junit"]
+
+RUN go get github.com/wadey/gocovmerge &&\
+ go get -u github.com/jstemmer/go-junit-report &&\
+ go get -v ./...
Review comment:
do you need to rerun`go get ./...` here since it was run in the base layer?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] lbathina commented on issue #4260: Codecov
Posted by GitBox <gi...@apache.org>.
lbathina commented on issue #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#issuecomment-571825331
Just saw. Will review it later tonight or tomorrow morning.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r376472178
##########
File path: tools/golang/docker-compose.yml
##########
@@ -32,4 +32,4 @@ services:
dockerfile: tools/golang/Dockerfile
target: unit
volumes:
- - ../..:/go/src/github.com/apache/trafficcontrol
Review comment:
Couple reasons
1. Because the Dockerfile is already `ADD`ing the whole context to that path.
2. Using that volume meant that some of the temporary files being created were being output into the source directory owned by `root:root` with permissions `-rw-------` and in general if we did things like `go fmt` it'd modify the actual source when ran. Seemed dangerous and - given the first point - unnecessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r376574305
##########
File path: tools/golang/docker-compose.yml
##########
@@ -32,4 +32,4 @@ services:
dockerfile: tools/golang/Dockerfile
target: unit
volumes:
- - ../..:/go/src/github.com/apache/trafficcontrol
Review comment:
Why would you use Docker to use it locally?
I clocked the `go get` step at 20 seconds running locally, which I wouldn't consider 'forever' but also note that it would still need to do that step if the volume still existed. The only thing that could be faster is that `ADD` line for the repository that doesn't need to exist if it's in a volume, and I didn't put that in there. Not sure why it was in there, tbh.
So deleting this line didn't make anything slower - although that doesn't mean it couldn't help it be faster.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] mhoppa commented on issue #4260: Codecov
Posted by GitBox <gi...@apache.org>.
mhoppa commented on issue #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#issuecomment-583151007
:shipit:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] mhoppa commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r376478336
##########
File path: tools/golang/docker-compose.yml
##########
@@ -32,4 +32,4 @@ services:
dockerfile: tools/golang/Dockerfile
target: unit
volumes:
- - ../..:/go/src/github.com/apache/trafficcontrol
Review comment:
still takes forever and will probably make me not use it locally :/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] mhoppa commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r376579798
##########
File path: tools/golang/docker-compose.yml
##########
@@ -32,4 +32,4 @@ services:
dockerfile: tools/golang/Dockerfile
target: unit
volumes:
- - ../..:/go/src/github.com/apache/trafficcontrol
Review comment:
I like/prefer using docker locally for my work as it matches what will be seen in ci and will be consistent but thats personally preference.
We dont change our dependencies much so my thought of putting it together was I would have to just build the container once in awhile as dependencies change and be able to quickly verify all tests in the repo are passing with passing in the current code as a volume.
my review isnt really voting anywho 🤷♂ so you can leave it as is
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r363532392
##########
File path: tools/golang/Dockerfile
##########
@@ -42,4 +42,10 @@ CMD ["golangci-lint", "run", "./..."]
FROM base AS unit
-CMD sh -c 'go test $(go list ./... | grep -v -e experimental -e "test/router" -e "api/v14")'
+VOLUME ["/junit"]
+
+RUN go get github.com/wadey/gocovmerge &&\
+ go get -u github.com/jstemmer/go-junit-report &&\
+ go get -v ./...
Review comment:
Was it? I didn't realize. Thought the linter ran that. I can take this out.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] lbathina commented on issue #4260: Codecov
Posted by GitBox <gi...@apache.org>.
lbathina commented on issue #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#issuecomment-572160357
can we have the coverage post somewhere for review?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] mhoppa commented on issue #4260: Codecov
Posted by GitBox <gi...@apache.org>.
mhoppa commented on issue #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#issuecomment-582942489
would like to get this merged so we can replace the duplicated unit tests in the repo. @ocket8888 whats blocking this besides the merge conflict? A committer?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rawlinp merged pull request #4260: Codecov
Posted by GitBox <gi...@apache.org>.
rawlinp merged pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on issue #4260: Codecov
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on issue #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#issuecomment-572161529
I can paste it into the #dev channel on the ATC slack, or put it somewhere on the wiki.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] mhoppa commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r376469499
##########
File path: tools/golang/docker-compose.yml
##########
@@ -32,4 +32,4 @@ services:
dockerfile: tools/golang/Dockerfile
target: unit
volumes:
- - ../..:/go/src/github.com/apache/trafficcontrol
Review comment:
wait why the removal of this volume?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r376474625
##########
File path: tools/golang/docker-compose.yml
##########
@@ -32,4 +32,4 @@ services:
dockerfile: tools/golang/Dockerfile
target: unit
volumes:
- - ../..:/go/src/github.com/apache/trafficcontrol
Review comment:
Well it shouldn't be `go get`-ing *everything* anymore - that was actually causing problems because it was fetching vendored dependencies (and some of ours are so old they don't exist anymore). So now it should only be fetching things from `golang.org/x/`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r376581333
##########
File path: tools/golang/docker-compose.yml
##########
@@ -32,4 +32,4 @@ services:
dockerfile: tools/golang/Dockerfile
target: unit
volumes:
- - ../..:/go/src/github.com/apache/trafficcontrol
Review comment:
Ohh, I see. Well lemme ask you this: why do we need the `ADD` at all if there's going to be a volume? Do we? Can I just get rid of it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] mhoppa commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r376472997
##########
File path: tools/golang/docker-compose.yml
##########
@@ -32,4 +32,4 @@ services:
dockerfile: tools/golang/Dockerfile
target: unit
volumes:
- - ../..:/go/src/github.com/apache/trafficcontrol
Review comment:
well it makes me rebuild the container everytime I want to run it locally which is super annoying as it takes forever to build since we do `go get`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] lbathina edited a comment on issue #4260: Codecov
Posted by GitBox <gi...@apache.org>.
lbathina edited a comment on issue #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#issuecomment-572160357
can we have the coverage post somewhere for review?
Goal is for us to view at the % and improve
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r376617190
##########
File path: tools/golang/docker-compose.yml
##########
@@ -32,4 +32,4 @@ services:
dockerfile: tools/golang/Dockerfile
target: unit
volumes:
- - ../..:/go/src/github.com/apache/trafficcontrol
Review comment:
No actually we can't unless we move the `go get`s to runtime. Because the volume isn't available during the build.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on issue #4260: Codecov
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on issue #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#issuecomment-571730865
Thanks, still want a review from @lbathina to make sure it fits her original use case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] mhoppa commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r363749970
##########
File path: tools/golang/Dockerfile
##########
@@ -42,4 +42,10 @@ CMD ["golangci-lint", "run", "./..."]
FROM base AS unit
-CMD sh -c 'go test $(go list ./... | grep -v -e experimental -e "test/router" -e "api/v14")'
+VOLUME ["/junit"]
+
+RUN go get github.com/wadey/gocovmerge &&\
+ go get -u github.com/jstemmer/go-junit-report &&\
+ go get -v ./...
Review comment:
yep! the base layer runs it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r376472178
##########
File path: tools/golang/docker-compose.yml
##########
@@ -32,4 +32,4 @@ services:
dockerfile: tools/golang/Dockerfile
target: unit
volumes:
- - ../..:/go/src/github.com/apache/trafficcontrol
Review comment:
Couple reasons
1. Because the Dockerfile is already `ADD`ing the whole context to that path.
2. Using that volume meant that some of the temporary files being created were being output into the source directory owned by `root:root` with permissions `-rw-------` and in general if we did things like `go fmt` it'd modify the actual source when ran. Seemed dangerous.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] mhoppa commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r376586156
##########
File path: tools/golang/docker-compose.yml
##########
@@ -32,4 +32,4 @@ services:
dockerfile: tools/golang/Dockerfile
target: unit
volumes:
- - ../..:/go/src/github.com/apache/trafficcontrol
Review comment:
Ahh I get what your saying now :) yeah we should be able to remove the ADD since its a volume.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on issue #4260: Codecov
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on issue #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#issuecomment-583149009
There were still some issues running it with Docker, so I've fixed those now. Posted the results of running it against master at the time of this writing in the #general channel of the ATC slack. Should've gone in #dev, but I didn't think of that until looking at the comments here again. Whoops.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r363507313
##########
File path: tools/golang/docker-compose.yml
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
---
-version: '2.4'
+version: '2.3'
Review comment:
Because the version of docker engine available from "dockerrepo" on CentOS7 isn't compatible with `docker-compose.yml` version 2.4 - and the contents are perfectly compatible with 2.3 (apparently).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] mhoppa commented on a change in pull request
#4260: Codecov
Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4260: Codecov
URL: https://github.com/apache/trafficcontrol/pull/4260#discussion_r363500140
##########
File path: tools/golang/docker-compose.yml
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
---
-version: '2.4'
+version: '2.3'
Review comment:
why the downgrade?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services