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 missing check options and missing checks #178

Merged
merged 1 commit into from
Sep 23, 2021

Conversation

DisasteR
Copy link

@DisasteR DisasteR commented Jul 30, 2021

needed some http options. ended adding all missing checks and check options

evrardjp
evrardjp previously approved these changes Aug 2, 2021
Copy link
Owner

@evrardjp evrardjp left a comment

Choose a reason for hiding this comment

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

Hello,

I am not having much time to tackle that this week, and I am on vacation after that.

So I will not be able to verify if everything is okay in terms of templating, and whether those options make sense.

There are two ways to go forward:

  • Either I blindly trust you (as it seems good enough)
  • Or we add a test for covering your use case, which actively tests the behaviour.

The former is the faster for everyone. However, it also means it could break in the future, as we are not testing this. I am not using this rserver part of the template myself and I never claimed to support it myself ;) (it's not documented).

So if you're willing to step up and add some tests, please tell me: I will then wait for the addition of the tests to merge this PR. Otherwise, I can merge now when the current tests are passing.

Please advice :)

@evrardjp
Copy link
Owner

evrardjp commented Aug 2, 2021

The CI failure seems unrelated. I will push a fix, and rebase this.

@evrardjp
Copy link
Owner

evrardjp commented Aug 2, 2021

Sadly the fix will take more time than I expected, so I will not be able to fix this very soon. Sorry for the inconvenience. I will do my best to fix that in the next weeks.

@DisasteR
Copy link
Author

DisasteR commented Aug 2, 2021

I am currently using some of this New options and i am confident about the others.
My opinion is merge it and if needed others will PR as i have done ^^

@evrardjp
Copy link
Owner

evrardjp commented Aug 17, 2021

I will have a look when I am back from vacation.

And, yes, totally agree. It's just that the CI is borked because an upstream project for building images is broken. I will have to switch to another one. But that's another convo :)

Repository owner deleted a comment from johnsmithdoe970 Aug 17, 2021
@evrardjp
Copy link
Owner

I am back from vacation. I will work on that today or tomorrow.

@evrardjp
Copy link
Owner

Hello,

It seems I don't have access to push to your branch. Do you mind either allowing me, or to rebase this PR from master branch, please?

@evrardjp evrardjp dismissed their stale review September 23, 2021 12:12

Need rebase.

@DisasteR
Copy link
Author

@evrardjp done :)

@evrardjp
Copy link
Owner

Thanks. Go go github actions!

@evrardjp evrardjp merged commit cc1d2f4 into evrardjp:master Sep 23, 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.

None yet

2 participants