Skip to content

Conversation

@ffaraone
Copy link
Collaborator

@ffaraone ffaraone commented Nov 22, 2024

Description

Related issue number

Special notes

Checklist

  • The pull request title is a good summary of the changes
  • Unit tests for the changes exist
  • New and existing unit tests pass locally

nk-hystax and others added 10 commits November 20, 2024 14:43
## Description

Ability to update cluster with the latest release

## Related issue number

OS-7958

## Checklist

* [ ] The pull request title is a good summary of the changes
* [ ] Unit tests for the changes exist
* [ ] New and existing unit tests pass locally
c41484f OS-4648. Update archived recommendations empty message and add
page description
60fdaab OS-7987. Add the port to the endpoint_url parameter in the
Profiling side modal
c3b4b18 OS-7959. Power schedule events
ea7b1b2 OS-8011. Update profiling integration examples
ccc2e55 OS-7958. Ability to update cluster with the latest release
c4b8875 OS-8011. Update Profiling integration side modal
850cd7b OS-8007. Add GCP filter support for InstancesForShutdown and ShortLivingInstances recommendations
046c2f3 OS-8003. Updated aiohttp version
def check_version(self):
if self.version.lower() == LATEST_TAG:
self.version = subprocess.check_output(
GET_LATEST_TAG_CMD, shell=True).decode("utf-8").rstrip()

Check failure

Code scanning / Bandit

subprocess call with shell=True identified, security issue. Error

subprocess call with shell=True identified, security issue.

Choose a reason for hiding this comment

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

I think it's safe to dismiss this alert as GET_LATEST_TAG_CMD always evaluates to a static string, so no risk for injection with a user string.

I suppose they added this due to the pipe in the command -- this is is not strictly necessary as the output of curl can be parsed in python instead of jq but that's how hystax devs implemented it and I'd rather keep it that way to keep future syncs as easy as possible

Choose a reason for hiding this comment

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

Actually now looking through the changes we've made in our fork (hystax/optscale@integration...softwareone-platform:optscale:main) a lot of them are adding usedforsecurity=False in hashlib.md5 calls - @ffaraone was this done to suppress similar bandit issues?

While these are great changes maybe we should rather create PRs to hystax/optscale directly about them, so that:

  1. We get fewer merge conflicts whenever we're doing a sync, and
  2. We solve the issue at the source, I'm sure Hystax will appreciate this

It's weird our pipeline caught this but not theirs, bandit is a very popular and useful tool, so to go even further we could open an issue on their repo to hear their thoughts about integrating it into their pipeline. That way changes like this won't be pushed in the first place which works best for everyone.

@ffaraone @antoniodimariano what do you think?

Copy link

@arturbalabanov arturbalabanov left a comment

Choose a reason for hiding this comment

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

Other than the discussion I opened (left it unresolved for visibility, feel free to resolve it again), the changes look good :)

@ffaraone ffaraone merged commit bea80b9 into softwareone-platform:main Nov 25, 2024
7 of 11 checks passed
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.

8 participants