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

Support more sophisticated proxy settings #52

Closed
wants to merge 1 commit into from

Conversation

mattesh
Copy link

@mattesh mattesh commented Feb 19, 2024

The commit 17f2ee0
from gwinkless was giving me the right direction, but was not sufficient for my Kerberos based proxy system.
I added required parameters to treat curl as required for different constellations, trying to do as minimal changes to the rest.
@donho and @gwinkless could you check from your end if this changes are OK to release?
Regards, Mattes

@Neustradamus
Copy link

@donho: What do you think?

@mattesh
Copy link
Author

mattesh commented Aug 23, 2024

@donho,
Is there anything blocking this to be merged?
We did tests and see this as crucial change for new releases of NPP+ Updater
Please, let me know if something should be added.
Thank you
/BR Mattes

const std::wstring& getAuth() const { return _proxyAuth; };
void setAuth(const std::wstring& s) { _proxyAuth = s; };
const std::wstring& getUserPass() const { return _proxyUserPass; };
void setUserPass(const std::wstring& s) { _proxyUserPass = s; };
Copy link
Member

Choose a reason for hiding this comment

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

User password shouldn't be stored into the xml file.

@@ -146,7 +146,7 @@
<ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
<ClCompile>
<Optimization>Disabled</Optimization>
<AdditionalIncludeDirectories>..\src\TinyXml;..\src\sha2;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
Copy link
Member

Choose a reason for hiding this comment

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

Why is this file modified?


curl_easy_setopt(curl, CURLOPT_PROXYAUTH, lAuth); // any method as proposed by proxy
}
if (!extraOptions.getUserPass().empty()) {
Copy link
Member

Choose a reason for hiding this comment

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

Coding convention is not respected.

@donho
Copy link
Member

donho commented Sep 4, 2024

The KISS (Keep it Simple, Stupid) guideline should be respected.
However, this PR is complex than it should be, also there are so many stuff added into UI which makes UX difficult.
Therrefore the PR is not accepted.

@donho donho closed this Sep 4, 2024
@donho donho added the reject label Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants