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.

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

Enable builds on all branches #383

Merged
merged 3 commits into from
Aug 25, 2021
Merged

Enable builds on all branches #383

merged 3 commits into from
Aug 25, 2021

Conversation

Excaleo
Copy link
Contributor

@Excaleo Excaleo commented Aug 24, 2021

I keep forgetting release builds have their own separate branch, previous attempt at limiting builds ended up disabling release builds.

I have turned off the build on pull request option, since building on every commit to a pushed branch would replace it. This prevents the build from building twice per PR.

chenzhihao
chenzhihao previously approved these changes Aug 25, 2021
@tiedotguy
Copy link
Collaborator

Closes #280

@Excaleo
Copy link
Contributor Author

Excaleo commented Aug 25, 2021

@tiedotguy I've also increased the http forwarder test timeout duration to 1.5 seconds as the ARM build sometimes times out in Travis CI, is this an acceptable test degredation for the AMD64 build?

1.5 seconds was derived from the previous failing travis build which indicated a duration of 1.3s

@Excaleo Excaleo requested a review from tiedotguy August 25, 2021 00:52
@tiedotguy
Copy link
Collaborator

If we've got tests timing out, that's an indication of a problem somewhere - the purpose of the timeout is to say "hey, this test is slow, if you need to perform a sleep in order to avoid 'raceyness', then the test should be refactored"

It's fine to bump it if it's necessary, but we should give a failing test a sniff test to see if it's got bad behavior, and raise an issue to address it if necessary.

@tiedotguy
Copy link
Collaborator

You can also merge without coveralls passing, but give it a quick look and see if there's an actual flakeyness problem, and raise an issue if so - in this case, the report doesn't say anything went down, so I think it's fine. Not certain why it's shown as a decrease in GH.

@Excaleo
Copy link
Contributor Author

Excaleo commented Aug 25, 2021

If we've got tests timing out, that's an indication of a problem somewhere - the purpose of the timeout is to say "hey, this test is slow, if you need to perform a sleep in order to avoid 'raceyness', then the test should be refactored"

From this build result, it seems this is a degradation within the ARM compiled binary. https://app.travis-ci.com/github/atlassian/gostatsd/jobs/533596613#L843

BenchmarkReceive-80 1 1228779055 ns/op 262749480 B/op 12341 allocs/op

The same benchmark in the AMD build takes a much lower time.

BenchmarkReceive-2 105783 11717 ns/op 18962 B/op 4 allocs/op

Might be worth raising this as a separate issue, I think the test case itself is fine.

@tiedotguy
Copy link
Collaborator

tiedotguy commented Aug 25, 2021

That's a lot of nanoseconds. Please raise an issue, it should be looked in to.

edit: but it's not a blocker

@Excaleo
Copy link
Contributor Author

Excaleo commented Aug 25, 2021

Issue #384 has been raised, going ahead with this merge to patch the security vulnerability.

@Excaleo Excaleo merged commit 717c30e into master Aug 25, 2021
@tiedotguy tiedotguy deleted the enable-all-branch-builds branch August 16, 2022 04:26
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.

3 participants