-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add Flag for 8.1.0.0 release #294
Conversation
@@ -4,6 +4,7 @@ const { loadRules } = require("./rulesUtils"); | |||
let info = console.info; | |||
beforeEach(() => { | |||
console.info = () => {}; | |||
process.env.CURAM_VERSION="8.3"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great nice that we can test this
@@ -5,7 +5,11 @@ const filesAndFolders = require("./filesAndFolders"); | |||
*/ | |||
const loadRules = config => { | |||
const rulesJson = []; | |||
const rulesFiles = filesAndFolders.glob(`${config.cssRulesTool.rulesFolder}/*.json`); | |||
if(process.env.CURAM_VERSION == "8.1.0.0"){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will work fine for this release but is not ideal from a maintenance perspective! For example for 8.3.0 or 8.2.1.0, we have to make this an allowlist with valid versions and we would have to update the list for each release!
There are actually 2 ways to handle this! The first is an allowlist like described above and have to maintain that list. A better way is to examine the second digit of CURAM_VERSION. So for 8.1.0, the 2nd digit is 1, for 8.2.0, it is 2 e.t.c. Just check that 2nd digit is greater than 0. For V9 this logic may have to change but you could validate that first digit is 8.
By the way it might also be worth doing a validation check using a regular expression to ensure that the CURAM_VERSION is in the pattern 8.{0-9}.{0-9}.{0-9}. You can use a regular expression service to some up with pattern : https://www.regextester.com/. SHould be quite easy actually:-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good Luiza. Will work fine for 8.1.0.0. Only thing is we can improve the logic as per my comment so that we don't have to maintain list of versions going forward:-)
@@ -5,7 +5,15 @@ const filesAndFolders = require("./filesAndFolders"); | |||
*/ | |||
const loadRules = config => { | |||
const rulesJson = []; | |||
const rulesFiles = filesAndFolders.glob(`${config.cssRulesTool.rulesFolder}/*.json`); | |||
const curamVersion = process.env.CURAM_VERSION; | |||
const regex = new RegExp("[8].[1-9].[0-9].[0-9]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder will this Regex satisfy an IFix too? e.g 8.1.0.0_iFix1. As it is it might satisfy an iFix condition too or you might have to add a "." e.g new RegExp("[8].[1-9].[0-9].[0-9]."
No description provided.