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

fix: allow http requests towards code local engine [NEBULA-1541] #4934

Merged
merged 1 commit into from Nov 21, 2023

Conversation

soniqua
Copy link
Contributor

@soniqua soniqua commented Nov 16, 2023

What does this PR do?

  • Allows requests made towards the Snyk Code Local Engine /status endpoint to respect the protocol of the Local Engine URL set on the org/group
  • Fixes conditions where a Snyk Code Local Engine installation responds on both http and https and we want to query http only
  • Allows certificate trust to remain intact

Where should the reviewer start?

How should this be manually tested?

Any background context you want to provide?

What are the relevant tickets?

Screenshots

Additional questions

Copy link
Contributor

github-actions bot commented Nov 16, 2023

Warnings
⚠️

You've modified files in src/ directory, but haven't updated anything in test folder. Is there something that could be tested?

Generated by 🚫 dangerJS against 5adbbed

@soniqua soniqua marked this pull request as ready for review November 16, 2023 12:34
@soniqua soniqua requested a review from a team November 16, 2023 12:34
@soniqua soniqua force-pushed the fix/allow-scle-http branch 2 times, most recently from 2d15383 to aaa0f44 Compare November 17, 2023 08:58
@shirlupo
Copy link
Contributor

from the code seems like we already calling this function with baseURL

await logLocalCodeEngineVersion(baseURL);

my assumption is we do not,
can you maybe change this as well?

I am a bit confused what is happening here if no broker in the picture? are we just failing this as Snyk Code Local Engine health is not ok ?

Copy link
Contributor

@shirlupo shirlupo left a comment

Choose a reason for hiding this comment

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

Approved with some sug and a question

Copy link
Contributor

@neonnoon neonnoon left a comment

Choose a reason for hiding this comment

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

I am not sure I understand what you mean with this @shirlupo:

from the code seems like we already calling this function with baseURL
my assumption is we do not,

Do you mean, why do we set a default parameter (='')?
That is a fair question as this will clearly not work. So can localEngineUrl be undefined? In that case we'd have to properly handle it instead of defaulting to empty string.

I am a bit confused what is happening here if no broker in the picture? are we just failing this as Snyk Code Local Engine health is not ok ?

There is no broker in the picture either way. This is CLI connecting directly to local engine.
Where would you expect broker (server or client?) to be involved?

@shirlupo
Copy link
Contributor

I am not sure I understand what you mean with this @shirlupo:

from the code seems like we already calling this function with baseURL
my assumption is we do not,

Do you mean, why do we set a default parameter (='')? That is a fair question as this will clearly not work. So can localEngineUrl be undefined? In that case we'd have to properly handle it instead of defaulting to empty string.

I meant the naming of the parameter 'baseUrl'

await logLocalCodeEngineVersion(baseURL);

from Matt and Matthew's code it seems like they parse the baseUrl so calling the parameter here baseURL is odd to me


I am a bit confused what is happening here if no broker in the picture? are we just failing this as Snyk Code Local Engine health is not ok ?

There is no broker in the picture either way. This is CLI connecting directly to local engine. Where would you expect broker (server or client?) to be involved?

There is a call here to get the version by calling the health check no?

@neonnoon
Copy link
Contributor

I meant the naming of the parameter 'baseUrl'
from Matt and Matthew's code it seems like they parse the baseUrl so calling the parameter here baseURL is odd to me

Ok, I thin base isn't quite right (as it's the full localEngineUrl incl. /api)?
But why do you think it shouldn't be called baseUrl because it is parsed?

There is no broker in the picture either way. This is CLI connecting directly to local engine. Where would you expect broker (server or client?) to be involved?

There is a call here to get the version by calling the health check no?

Yes, directly to the local engine (via local-proxy), no broker is needed for this.

@soniqua
Copy link
Contributor Author

soniqua commented Nov 20, 2023

Yep, the parameter fed to this function includes the /api path, so baseUrl is a misnomer in analysis.ts.

@shirlupo
Copy link
Contributor

yeah I meant the naming "base" is not right, that's it not biggy :)

@soniqua soniqua enabled auto-merge (squash) November 21, 2023 16:14
@soniqua soniqua merged commit 92d88b6 into master Nov 21, 2023
12 checks passed
@soniqua soniqua deleted the fix/allow-scle-http branch November 21, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants