Correctly handle bytes when given milibytes input, add test coverage

heresandyboy commented 1 year ago

I noticed a number of my pod.spec.containers resource values were automatically converted from Gi to milibytes after deployment.

So in order to properly handle milibytes in Skooner I have fixed and refactored the function parseUnitsOfBytes()

Fixes issue #395

Explanation of refactors:

Added an optional 'm' at the end of the number in the regular expression: (m)?
Added a new variable isMiliBytes to check whether the input value is in milibytes.
Adjusted the conversion factors based on whether the input value is in milibytes.
Using array destructuring to extract the different parts of the regex match result.
Using a single regex with named capture groups, which makes the code more readable.
Returning early if the input value is falsy, to avoid unnecessary computation.
Calculating the conversion factor in a single step, using the unit index, exponent, and miliBytes flag.
Using a ternary operator to set the unitFactor variable based on whether the unit has an "i" suffix or not.

Added a bunch of tests

There are very few tests in this project. I created a bunch of tests covering the file before working in order to better understand the code and allow me to refactor

Example of the initial issue from k8s resources:

Pre-deployment values:

        resources:
          limits: 
            memory: 1.4Gi
          requests:
            memory: 1.4Gi

Post deployment values taken from within the cluster.

      resources:
        limits:
          memory: 1503238553600m
        requests:
          memory: 1503238553600m

This manifested as incorrect calculations in Skooner
- Here 1.4Ti should have been 1.4Gi
- Here 4200Gb should be 4.2Gb

heresandyboy commented 1 year ago

Hi, is anyone able to trigger workflows on this PR for me please? We'd really like to use Skooner but there is this, and several other bugs, I'd like to fix up before we can use it properly.

Hopefully one of the listed maintainers can help get things moving on this one, and reassure me enough to spend the time fixing other issues too.

Edit, it looks like I also need to add a Signed Off By to my commits. Who's email can I use for that? Frustratingly I see that I forgot to swap my github email from my work email when checking in on a new laptop. Hopefully that wont cause a problem?

@herbrandson @tianni4104 @chargao @yuqiuw @jeffhem @jadeapplegate

tianni4104 commented 1 year ago

@heresandyboy Thanks for your contribution. The pipeline seems to fail. Do you want to fix it?

heresandyboy commented 1 year ago

No problem, I'll have a good look in the morning. Interesting error, since I only added code, without changing any packages. The package.lock.json did regenerate though. I'll review it properly tomorrow.

npm ERR! Cannot read property '@types/humanize-duration' of undefined

npm ERR! A complete log of this run can be found in:
npm ERR!

heresandyboy commented 1 year ago

Ah, it looks like your ci is still using node v12.22.12 which was EOL last April, my package.lock was generated with the latest LTS. I think what I might do is either reset the package.lock.json back to previous, or gen it again using the old version of node. That'll keep this PR simple. Then we can look at upgrading to node 18.12.0 LTS separately.

tianni4104 commented 1 year ago

It's a good plan for me

heresandyboy commented 1 year ago

Apologies I have had an exceptionally busy time with work at the moment, I'll get on to this as soon as I can.

heresandyboy commented 1 year ago

I misunderstood the sign off process there, fixed it eventually. This should be ready for review etc now thanks.

heresandyboy commented 1 year ago

@tianni4104 this is ready to trigger workflows now, when you get a minute.

heresandyboy commented 1 year ago

@tianni4104 Thanks for running the workflows this morning, I have fixed the linting issues so you can trigger again, unfortunately the lint scripts in the client package.json don't work with latest node/eslint any more. I had to manually fix the lint issues.

Linting error locally:

$ npm run lint

> @skooner-k8s/skooner-client@0.1.0 lint
> tsc --noEmit && eslint 'src/**/*.{js,jsx,ts,tsx}' --fix

Oops! Something went wrong! :(

ESLint: 6.8.0.

No files matching the pattern "'src/**/*.{js,jsx,ts,tsx}'" were found.
Please check for typing mistakes in the pattern.

If we can get this PR boxed off, I will submit another PR to get everything up to date to make it easier to work with Skooner in the future.

tianni4104 commented 1 year ago

@heresandyboy Thanks. That's so awesome! Appreciate all your contribution.

heresandyboy commented 1 year ago

@tianni4104 I didn't spot those errors in the previous run, should have those fixed now

heresandyboy commented 1 year ago

Yep this is why we automate linting fixes hey, I was going to knock up a dev container with the old versions of everything to work in, but keep thinking this must be the last lint error!

This must be the last lint error now :)

skooner-k8s / skooner