Skip to content

Add support for ccf flag for O11y #779

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

Merged
merged 1 commit into from
Feb 22, 2024
Merged

Add support for ccf flag for O11y #779

merged 1 commit into from
Feb 22, 2024

Conversation

amaanbs
Copy link
Collaborator

@amaanbs amaanbs commented Feb 15, 2024

No description provided.

@amaanbs amaanbs requested a review from a team as a code owner February 15, 2024 15:30
Copy link
Collaborator

@karanshah-browserstack karanshah-browserstack left a comment

Choose a reason for hiding this comment

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

Can see this can impact other stuff too

@@ -61,6 +61,9 @@ module.exports = function run(args, rawArgs) {
markBlockStart('setConfig');
logger.debug('Started setting the configs');

// set cypress config filename
utils.setCypressConfigFilename(bsConfig, args);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move our code in end in this scenario?

Copy link
Collaborator Author

@amaanbs amaanbs Feb 15, 2024

Choose a reason for hiding this comment

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

That would require moving more code around. This change shouldn't affect handling of other products. This method sets the value of cypress_config_file in bsConfig which is only accessed by O11y helper method amongst all methods placed between this line & the earlier placement of this function call.

Copy link
Contributor

@rev-doshi rev-doshi left a comment

Choose a reason for hiding this comment

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

This same changes is also needed for A11y AFAIR, can you test that once ?

@amaanbs
Copy link
Collaborator Author

amaanbs commented Feb 16, 2024

Tested, working fine.

@pranavj1001 pranavj1001 merged commit 96a7f5a into master Feb 22, 2024
@pranavj1001 pranavj1001 mentioned this pull request Feb 22, 2024
@karanshah-browserstack karanshah-browserstack deleted the SDK-410 branch February 22, 2024 11:59
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.

6 participants