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

Test Apache license on Windows #2470

Merged
merged 1 commit into from Oct 1, 2020
Merged

Conversation

k-rus
Copy link
Contributor

@k-rus k-rus commented Sep 30, 2020

Fixes the regression test of Apache license on Windows.

PR note

GH is confused by some reason and shows difference on number of unchanged lines.

#2435 removed the Apache license test on Windows. This PR fixes it back.

@codecov
Copy link

codecov bot commented Sep 30, 2020

Codecov Report

Merging #2470 into master will increase coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2470      +/-   ##
==========================================
+ Coverage   90.04%   90.15%   +0.10%     
==========================================
  Files         212      212              
  Lines       34174    34124      -50     
==========================================
- Hits        30772    30764       -8     
+ Misses       3402     3360      -42     
Impacted Files Coverage Δ
tsl/src/fdw/shippable.c 82.85% <0.00%> (-11.43%) ⬇️
src/loader/bgw_message_queue.c 84.51% <0.00%> (-2.59%) ⬇️
src/import/planner.c 70.30% <0.00%> (+11.12%) ⬆️

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 cdb4c8f...b8e60b5. Read the comment docs.

@k-rus k-rus changed the title Test license on Windows Test Apache license on Windows Oct 1, 2020
@k-rus k-rus marked this pull request as ready for review October 1, 2020 07:27
@k-rus k-rus requested a review from a team as a code owner October 1, 2020 07:27
@k-rus k-rus requested review from pmwkaa, mkindahl, WireBaron, erimatnor and a team and removed request for a team October 1, 2020 07:27
@pmwkaa
Copy link
Contributor

pmwkaa commented Oct 1, 2020

@k-rus Funny that I've tried exact same approach, did not worked for some reason, probably missed something. Hope it will now.

The changed lines might be to the reason that Windows text files are usually use different end of line chars (\r\n vs \n). Maybe they have changed accidentally?

Copy link
Contributor

@erimatnor erimatnor left a comment

Choose a reason for hiding this comment

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

There's a lot of lines that seem unrelated to the changes in this PR, which makes it harder to review. I actually only identified three lines that seem relevant. Would be good to keep formatting as a separate change.

@k-rus
Copy link
Contributor Author

k-rus commented Oct 1, 2020

There's a lot of lines that seem unrelated to the changes in this PR, which makes it harder to review. I actually only identified three lines that seem relevant. Would be good to keep formatting as a separate change.

@erimatnor See my not in #2470 (comment)
I haven't done any changes to these lines. It might be due to git settings, I am checking it.

Copy link
Contributor

@mkindahl mkindahl left a comment

Choose a reason for hiding this comment

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

Looks good, but would be good to just have changed lines. There is no difference between some of the changed lines, so I suspect that it is a change in line ending as a result of the editor being helpful.

Copy link
Contributor

@pmwkaa pmwkaa left a comment

Choose a reason for hiding this comment

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

Apparently license tests passed, so this change makes sense. Yet there are some odd issues with other tests, likely not related to this PR

@k-rus
Copy link
Contributor Author

k-rus commented Oct 1, 2020

@pmwkaa @erimatnor @mkindahl I made a comment in the OP of the PR regarding unrelated lines. I guess nobody reads it:

GH is confused by some reason and shows difference on number of unchanged lines.

One explanation can be that git silently fixes the line endings. I have not set the configuration, so it is default behaviour.

@k-rus
Copy link
Contributor Author

k-rus commented Oct 1, 2020

Apparently license tests passed, so this change makes sense. Yet there are some odd issues with other tests, likely not related to this PR

@pmwkaa You are right the failures are not related to the PR, so I created an issue for the deterministic failures.

Funny that I've tried exact same approach, did not worked for some reason, probably missed something. Hope it will now.

You might not put in exactly the same place. It needs to be before the PG service is started or restarted.

Fixes the regression test of Apache license on Windows.
@k-rus k-rus merged commit 7f98d7f into timescale:master Oct 1, 2020
@k-rus k-rus deleted the windows-test-license branch October 1, 2020 10:56
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.

None yet

4 participants