Skip to content
This repository has been archived by the owner on Apr 28, 2020. It is now read-only.

Align vmware secret with DNS 1123 #549

Merged
merged 1 commit into from
Sep 11, 2019

Conversation

mareklibra
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Sep 10, 2019

Pull Request Test Coverage Report for Build 2311

  • 14 of 15 (93.33%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 82.821%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/k8s/requests/v2v/startV2VvmwareController.js 0 1 0.0%
Totals Coverage Status
Change from base Build 2304: 0.05%
Covered Lines: 4202
Relevant Lines: 4858

💛 - Coveralls


// starts with alphaNum
if (!str.charAt(0).match(alphanumericRegex)) {
return alignWithDNS1123(str.slice(1));
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be done much easier without a recursion

str.replace(/^[^a-zA-Z0-9]*/, '').replace(/[^a-zA-Z0-9]*$/, '')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, but I like recursion :-)

I can change it.

/**
* Based on V2V Provider Pod manifest.yaml
*/

const MAX_LEN = 30;

// TODO: this needs to be improved to conform validations.js::validateDNS1123SubdomainValue()
const alignWithDNS1123 = str => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we rename it to trimAccordingToDNS1123 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention is to further improve this method to fully conform DNS 1123, not just start and end. This first version was meant as a quick fix to unblock testing immediately. So it makes sense to keep the name as it is.

@rawagner rawagner merged commit 7485583 into kubevirt:master Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants