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

Add report-file argument #11

Closed
wants to merge 1 commit into from
Closed

Add report-file argument #11

wants to merge 1 commit into from

Conversation

WesleyKlop
Copy link

When trying to use this action on one of my projects I ran into the issue where the action could not find the coverage file, this fixes that

@WesleyKlop WesleyKlop requested a review from a team April 21, 2020 13:45
@franciscodua
Copy link
Contributor

Hi @WesleyKlop
Thanks for this amazing PR! I bet it will be useful to many people using it.
As an extra, could you just update the README to make it clear that we now have a new workflow option?

That would help people with the same needs find the solution much quicker.

@WesleyKlop
Copy link
Author

Ofcourse, I'll add it this weekend :)

@andreaTP
Copy link

Hi @WesleyKlop I hope I'm in time to avoid wasting your time!
Although we really appreciate your effort we decided to have a more generic approach to the problem in order to avoid re-binding all the option's matrix of the codacy-coverage-reporter tool.

On the bright side I'm going to test out and eventually merge this WIP:
#12
totally based on your approach.

Thanks a lot for your effort, very much appreciated!

@WesleyKlop
Copy link
Author

Hi @WesleyKlop I hope I'm in time to avoid wasting your time!
Although we really appreciate your effort we decided to have a more generic approach to the problem in order to avoid re-binding all the option's matrix of the codacy-coverage-reporter tool.

That's okay!

On the bright side I'm going to test out and eventually merge this WIP:
#12
totally based on your approach.

Thanks a lot for your effort, very much appreciated!

No problem! Isn't this what open source is about? :)

I did look into the other PR but can't it be argued that it's better to be explicit as to what options you're providing the users? That way there's also less possibility of making mistakes, with the downside of having more maintenance of this action.

Also, a bit off-topic but I hope that's okay, I was wondering why the docker image always downloads a new version of the codacy script instead of it being embedded at build time? This way the functionality of the docker container is not consistent and predictable.

Anyway thanks for the comments and I hope to be able to use a newer version for my projects soon!

@andreaTP
Copy link

@WesleyKlop thanks for the comments!
Caching the reporter in the Docker image is a great suggestion tracked here: #13

You are correct into being explicit in the action, the reason is that we would need to map the entire matrix of arguments of the tool to cover all the use cases, creating as well a strong inter-dependency in between the two projects without much instruments to keep them aligned other than manual sync.
That's why we decided to go down the road of a less maintenance approach.

@WesleyKlop
Copy link
Author

Alright, I understand! Maybe there is a middle ground somewhere to be found (like the type of action and flags as different parameters) as that might lead to better self-documentation and readability! (thinking about things like partial reports)

Thanks you for all the information I really appreciate it :D

@andreaTP
Copy link

andreaTP commented Apr 24, 2020

Maybe there is a middle ground somewhere to be found

Sadly we failed to find it but we remain completely open to listen possible proposals!

We love to hear from our users and your feedback is super-appreciated!

@WesleyKlop WesleyKlop closed this Apr 25, 2020
@franciscodua
Copy link
Contributor

Hi @WesleyKlop
We just added a similar parameter to the one you proposed. You can check it in this PR.
You can find the latest release for this action here.

Now you can just send a list of reports using a comma separated list of paths to reports

@WesleyKlop
Copy link
Author

Hi @WesleyKlop
We just added a similar parameter to the one you proposed. You can check it in this PR.
You can find the latest release for this action here.

Now you can just send a list of reports using a comma separated list of paths to reports

That's great, thanks! :)

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

3 participants