8000 ci: deprecate AppVoyer integration by khos2ow · Pull Request #128 · git-chglog/git-chglog · GitHub
[go: up one dir, main page]

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

ci: deprecate AppVoyer integration #128

Merged
merged 1 commit into from
Mar 26, 2021
Merged

Conversation

khos2ow
Copy link
Collaborator
@khos2ow khos2ow commented Mar 23, 2021

What does this do / why do we need it?

With the migration to GitHub Action, there's no need to continue to use
AppVoyer to only test on Windows, as it's covered by corresponding new
jobs.

Check lists

  • Test passed
  • Coding style (indentation, etc)

Which issue(s) does this PR fix?

Fixes #126

@coveralls
Copy link
coveralls commented Mar 23, 2021

Pull Request Test Coverage Report for Build 688216325

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 76.724%

Totals Coverage Status
Change from base Build 684566457: 0.0%
Covered Lines: 1836
Relevant Lines: 2393

💛 - Coveralls

strategy:
matrix:
os: [macos-latest, windows-latest]
Copy link
8000 Collaborator Author

Choose a reason for hiding this comment

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

For feature parity, I've added the matrix build here, but imo it's an overkill. We can reliably only use unit on ubuntu-20.04 (or ubuntu-latest for that matter) which does also upload code coverage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is really cool that this is supported so easily. It does feel like it is overkill at this point, but it is a "nice to have". I am open to either option really.

@khos2ow khos2ow requested review from mavogel and clok March 23, 2021 18:15
@clok
Copy link
Collaborator
clok commented Mar 23, 2021

This is interesting (and makes sense in a way), but the unit tests on windows fail due to carriage return instead of newline. See: https://github.com/git-chglog/git-chglog/runs/2177927696?check_suite_focus=true#step:4:50

@clok
Copy link
Collaborator
clok commented Mar 24, 2021

So, do we update the test that is failing?

We could consider the OS when performing the expectation with the generated output. Or we could check that \r\n is in the output, then search/replace the output to perform the expectation check. Just some thoughts.

Or do we deprecate the windows test?

@clok
Copy link
Collaborator
clok commented Mar 24, 2021

PS - ❤️ the branch name

@khos2ow
Copy link
Collaborator Author
khos2ow commented Mar 24, 2021

Or we could check that \r\n is in the output, then search/replace the output to perform the expectation check.

That seems to be the issue, but this feels a bit weird to mutate the output based on the underlying OS. That said, I haven't had enough time since I opened this PR to delve into this. But my first feeling is that we should not have these.

output = strings.ReplaceAll(strings.TrimSpace(buf.String()), "\r\n", "\n")

Although the difference betwen LF and CRLF might still be relevant to some folks.

@khos2ow khos2ow marked this pull request as draft March 24, 2021 22:13
@khos2ow
Copy link
Collaborator Author
khos2ow commented Mar 24, 2021

So, it seems this issue is directly related to how Go manages line endings on different platforms. In other words \n works as is in console (even in Windows), the same goes for fmt.Println, but when it comes to read and write to the file Go will use LF or CRLF based on the underlying platform it's being executed on (golang/go#28822)

That said, now, I'm in favor of always normalizing to LF. Because the intended destination of the output is a markdown file to be checked in a repository and be shared with broader audience. With that there won't be any difference between running git-chglog on a Linux or macOS or Windows.

Also with this change there won't be any need to run the tests on separate platforms, as they will always behave the same way.

@clok
Copy link
Collaborator
clok commented Mar 24, 2021

Nice find @khos2ow. After your explanation, it makes sense that we would be seeing this.

I support normalizing to LF in our case. Your point about the desired destination of the tool output is spot on.

I vote for deprecating the Windows tests at the least. I recognize that Mac OS is close enough to Linux in a general sense, so it may be overkill to include a separate Mac OS test run. That said, if it is minimal impact, it would be nice to keep the Mac OS test run. Mac is a common development platform.

@khos2ow
Copy link
Collaborator Author
khos2ow commented Mar 25, 2021

That said, now, I'm in favor of always normalizing to LF.

On the second though, I want to take back my earlier suggestion. I now think we'd best not mutate line endings at all and leave it to discretion of user to decide where they want to run this and how the line ending should look like. That's mainly because of the followings:

  • it's computationally expensive (this has to be done line by line over a loop)
  • the process deals with both read a file and write to a file
    • to expand on it, if it were only read and then output to stdout (i.e. no io.Writer involved) I might have considered it
  • the choice is being completely removed from user and LF is being forced to them with no way to have CRLF

That said, and if we all agree to this, there won't be any need to run the tests on different platforms per se. They would behave correctly and accordingly on each of those as long as the tests are passing.

@clok
Copy link
Collaborator
clok commented Mar 25, 2021

the choice is being completely removed from user and LF is being forced to them with no way to have CRLF

To me, that's the critical piece that makes me feel like this is something we don't need to worry about in these tests.

If we had concerns about file permissions management or something that can differ drastically for each OS, I would have more concern.

With the migration to GitHub Action, there's no need to continue to use
AppVoyer to only test on Windows, as it's covered by corresponding new
jobs.

Signed-off-by: Khosrow Moossavi <khos2ow@gmail.com>
Copy link
Collaborator Author
@khos2ow khos2ow left a comment

Choose a reason for hiding this comment

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

In light of the discussion above, I've reverted back matrix test and removed normalizing s/\r\n/\n/ in tests (as it doesn't happen anywhere else in the code, and that seems a hack to make tests pass on Windows)

That said, actual AppVoyer integration needs to be disabled on repository level and corresponding status checks need to be disabled from "Branch protection rules" (which I do not have access to neither.)

@khos2ow khos2ow marked this pull request as ready for review March 25, 2021 23:24
Copy link
Collaborator
@clok clok left a comment

Choose a reason for hiding this comment

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

Thank you for this. I have disabled the appvoyer checks on the mainline branch. See the image below. I will remove the webhook integration from the repo as well. Great work @khos2ow !

image

@clok clok merged commit 34b9d5c into git-chglog:master Mar 26, 2021
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.

Proposal: drop AppVoyer in favor of GitHub Action
3 participants
0