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

Address devskim warnings #196

Merged
merged 6 commits into from
Sep 28, 2023
Merged

Address devskim warnings #196

merged 6 commits into from
Sep 28, 2023

Conversation

ladatz
Copy link
Contributor

@ladatz ladatz commented Sep 27, 2023

Addresses #190

Motivation and Context

Ignores TLS warning by adding Devskim ignore to http endpoints and localhost.

Description

Ignores TLS warning by adding Devskim ignore to http endpoints and localhost. This gets rid of some security warnings that are not truly an issue as they all point to localhost. Most of the "localhost" references are in test code, but some are in our examples, which we don't expect to be used in production. Customers would use their own endpoints for their applications. Also suppress some "TODOs" in code paths that we don't anticipate will have further development on them for now.

Note: the only remaining devskim alerts should be in files that aren't as easy to suppress (i.e. some js files that are marked "generated" and come from somewhere else)

Copy link

@ashbeitz ashbeitz left a comment

Choose a reason for hiding this comment

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

Some minor comments. Please take a look.

examples/common/src/chariott/api.rs Outdated Show resolved Hide resolved
tests/registry-e2e.rs Outdated Show resolved Hide resolved
ess/src/ess.rs Fixed Show fixed Hide fixed
@wilyle
Copy link

wilyle commented Sep 27, 2023

As a general comment: why not fix the issues that you can right now instead of making an issue for later? For example, the clippy warning that's being ignored suggests using a struct to encapsulate all of the args which is something that can be done relatively quickly

@ladatz
Copy link
Contributor Author

ladatz commented Sep 27, 2023

As a general comment: why not fix the issues that you can right now instead of making an issue for later? For example, the clippy warning that's being ignored suggests using a struct to encapsulate all of the args which is something that can be done relatively quickly

The short answer is that we have not decided what the future of this code is yet; it may be moved and/ or removed, so I will leave it as-is for now in its stable state. But I wanted to address most of the devskim alerts for the repo now because we have so much noise it's hard to tell what might be a real issue

Copy link

@ashbeitz ashbeitz left a comment

Choose a reason for hiding this comment

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

Approved.

@ladatz ladatz merged commit af31e89 into main Sep 28, 2023
12 checks passed
@ladatz ladatz deleted the ladatz/resolvedevskim branch September 28, 2023 13:09
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.

4 participants