Skip to content

Conversation

@arontsang
Copy link
Contributor

No description provided.

@nabsul
Copy link
Owner

nabsul commented Apr 8, 2025

Thanks for the contribution! I will try to get the change reviewed and merged in the next few weeks. Please feel free to remind me if you don't hear back by end of this month :-)

// Try searching for account before account creation.
// This is needed for the Venafi implementation of ACME
// since they do NOT follow RFC8555 Section 7.3.1
return await PostAsync<AcmeAccountResponse>(cfg.AcmeKey, uri, new { onlyReturnExisting = true }, nonce);
Copy link
Owner

Choose a reason for hiding this comment

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

I think instead of using exception handling to switch between the regular approach and what Vanafi needs, a configuration parameter would be better.

if (!resp.IsSuccessStatusCode)
{
var content = await resp.Content.ReadAsStringAsync();
AcmeError? error = null;
Copy link
Owner

Choose a reason for hiding this comment

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

This structure looks odd. Can it be written in a clearer way?

@nabsul
Copy link
Owner

nabsul commented Jun 1, 2025

@arontsang Thanks again for submitting this, but I don't think I can accept it in its current form. I left some more detailed comments in the code.

For this project I strongly value making small non-breaking changes. Ideally, new features and breaking changes should be controlled by a configuration parameter that is off by default.

public required string Type { get; init; }
public required string Detail { get; init; }
public int Status { get; init; }
} No newline at end of file
Copy link
Owner

Choose a reason for hiding this comment

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

also please fix styling issues as much as possible, like the missing new line at the end of this file.

Copy link
Owner

@nabsul nabsul left a comment

Choose a reason for hiding this comment

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

left some comments.

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.

2 participants