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

Various Helm Chart maintenance updates #158

Merged
merged 6 commits into from
Apr 13, 2023
Merged

Various Helm Chart maintenance updates #158

merged 6 commits into from
Apr 13, 2023

Conversation

jujaga
Copy link
Member

@jujaga jujaga commented Apr 12, 2023

Description

This PR addresses various Helm Chart related issues:

SHOWCASE-3145

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Currently rebased on top of #157 - please merge this branch to fold in both branches of work.

@jujaga jujaga added the bug Something isn't working label Apr 12, 2023
@jujaga jujaga self-assigned this Apr 12, 2023
@codeclimate
Copy link

codeclimate bot commented Apr 12, 2023

Code Climate has analyzed commit 1ede154 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 51.1% (0.0% change).

View more on Code Climate.

@github-actions
Copy link

Coverage Report

Totals Coverage
Statements: 43.59% ( 1615 / 3705 )
Methods: 32.14% ( 170 / 529 )
Lines: 51.1% ( 1064 / 2082 )
Branches: 34.83% ( 381 / 1094 )

@jujaga jujaga requested review from pbolduc and cberg-aot April 12, 2023 23:57
@pbolduc
Copy link
Contributor

pbolduc commented Apr 13, 2023

👀

@@ -2,7 +2,7 @@ const crypto = require('crypto');
const jestJoi = require('jest-joi');
expect.extend(jestJoi.matchers);

const { Permissions } = require('../../../src/components/constants');
const { EMAILREGEX, Permissions } = require('../../../src/components/constants');
Copy link
Contributor

@TimCsaky TimCsaky Apr 13, 2023

Choose a reason for hiding this comment

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

just wondering, if BCBox needs to validate the email address .. so that it only calls the COMS search endpoint once the email is valid.
is the problem that as the bcbox user types, before the email is valid, it is passing it to coms?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is part of Kyle's PR. The regex is being backported from BCBox to here so that it remains in alignment - the Joi email definition isn't sufficiently meeting our needs.

* Generic email regex modified to require domain of at least 2 characters
* @see {@link https://emailregex.com/}
*/
EMAILREGEX: '^[a-zA-Z0-9.!#$%&’*+/=?^_`{|}~-]+@[a-zA-Z0-9-]+(?:\\.[a-zA-Z0-9-]{2,})+$',
Copy link
Contributor

@bcgv123 bcgv123 Apr 13, 2023

Choose a reason for hiding this comment

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

Wondering would be better to define the constant like: "EMAIL_REGEX" instead as combined (one word)? In the future we will have a multiple word constant, it might look nicer to split by _

@TimCsaky TimCsaky merged commit 9e42119 into master Apr 13, 2023
@jujaga jujaga deleted the bugfix/helm-hpa branch April 13, 2023 18:18
@jujaga jujaga mentioned this pull request Apr 13, 2023
4 tasks
Copy link
Contributor

@bcgv123 bcgv123 left a comment

Choose a reason for hiding this comment

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

Wondering would be better to define the constant like: "EMAIL_REGEX" instead as combined (one word)? In the future we will have a multiple word constant, it might look nicer to split by _

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants