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.

Sign up for GitHub

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

Jump to bottom

Adding support for getting env config through client env #2838

Merged
merged 2 commits into from
Sep 9, 2024
Merged

Adding support for getting env config through client env #2838

merged 2 commits into from
Sep 9, 2024

Conversation

bandana147
Copy link
Contributor

@bandana147 bandana147 commented Sep 5, 2024 •
edited
Loading

@bandana147 bandana147 added run-nala Run Nala Test Automation against PR needs-verification PR requires E2E testing by a reviewer labels Sep 5, 2024
@bandana147 bandana147 requested a review from a team as a code owner September 5, 2024 11:34
@aem-code-sync AEM Code Sync
Copy link
Contributor

aem-code-sync bot commented Sep 5, 2024 •
edited
Loading

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

@aem-code-sync AEM Code Sync
Copy link
Contributor

aem-code-sync bot commented Sep 5, 2024 •
edited
Loading

Page Scores Audits Google
M /?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
D /?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@codecov Codecov
Copy link

codecov bot commented Sep 5, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.08%. Comparing base (264c998) to head (1f36940).
Report is 12 commits behind head on stage.

Additional details and impacted files 
@@            Coverage Diff             @@
##            stage    #2838      +/-   ##
==========================================
+ Coverage   95.89%   96.08%   +0.19%     
==========================================
  Files         173      215      +42     
  Lines       46316    53949    +7633     
==========================================
+ Hits        44415    51838    +7423     
- Misses       1901     2111     +210

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@spadmasa spadmasa assigned spadmasa and prativas22 and unassigned spadmasa Sep 5, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 6, 2024

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

@narcis-radu
Copy link
Contributor

@bandana147 - what's the use-case for this enhancement?

@bandana147
Copy link
Contributor Author

bandana147 commented Sep 6, 2024 •
edited
Loading

@bandana147 - what's the use-case for this enhancement?

@narcis-radu This applies to standalone gnav clients. If their page is not listed in this condition:

if (host.includes(${SLD}.page) || host.includes(${SLD}.live) || host.includes('stage.adobe') || host.includes('corp.adobe') || host.includes('graybox.adobe')) { return { ...ENVS.stage, consumer: conf.stage }; }
it always defaults to production.
We did suggest using the env URL parameter, but it seems to get lost sometimes during redirection.
They’ve requested the ability to pass the env parameter while configuring the navigation, to ensure they can consistently use the correct environment for their stage setup.

mokimo
mokimo approved these changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@mokimo mokimo left a comment

Choose a reason for hiding this comment

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

new config, new unit test!

libs/utils/utils.js Outdated Show resolved Hide resolved
@skumar09 skumar09 added run-nala Run Nala Test Automation against PR and removed run-nala Run Nala Test Automation against PR labels Sep 6, 2024
@prativas22
Copy link

The PR is validated, and the test results are updated in the ticket's subtask.
https://jira.corp.adobe.com/browse/MWPW-157474

https://adobecom.github.io/nav-consumer/navigation.html?navbranch=mwpw-157474&authoringpath=/federal/dev/gnavtest/ccgnav
image
image

@prativas22 prativas22 added verified PR has been E2E tested by a reviewer Ready for Stage and removed run-nala Run Nala Test Automation against PR needs-verification PR requires E2E testing by a reviewer labels Sep 9, 2024
@milo-pr-merge milo-pr-merge bot merged commit 8138b79 into stage Sep 9, 2024
27 of 28 checks passed
@milo-pr-merge milo-pr-merge bot deleted the MWPW-157474 branch September 9, 2024 16:09
@milo-pr-merge milo-pr-merge bot mentioned this pull request Sep 9, 2024
Merged
elan-tbx pushed a commit that referenced this pull request Sep 9, 2024
@bandana147 @elan-tbx
Adding support for getting env config through client env (#2838)
0e61d71 
* Adding support for getting env config through client env

* Attaching client env to window
narcis-radu
narcis-radu reviewed Sep 10, 2024
View reviewed changes
libs/navigation/navigation.js
@@ -37,6 +37,7 @@ export default async function loadBlock(configs, customLib) {
const {
header, footer, authoringPath, env = 'prod', locale = '', theme,
} = configs || {};
window.miloClientEnv = env;
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not export a global variable with environment details

salonijain3 added a commit that referenced this pull request Sep 10, 2024
@salonijain3
Revert "Adding support for getting env config through client env (#2838…
5a84c12 
…)"

This reverts commit 8138b79.
@mokimo mokimo mentioned this pull request Sep 10, 2024
Merged
mokimo added a commit that referenced this pull request Sep 10, 2024
@mokimo @salonijain3
Revert "Adding support for getting env config through client env" (#2858
2802011 
)

Revert "Adding support for getting env config through client env (#2838)"

This reverts commit 8138b79.

Co-authored-by: Saloni Jain <6162294+salonijain3@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@narcis-radu narcis-radu narcis-radu left review comments

@sonawanesnehal3 sonawanesnehal3 sonawanesnehal3 approved these changes

@sharmrj sharmrj sharmrj approved these changes

@mokimo mokimo mokimo approved these changes

@salonijain3 salonijain3 salonijain3 approved these changes

@overmyheadandbody overmyheadandbody Awaiting requested review from overmyheadandbody

@Deva309 Deva309 Awaiting requested review from Deva309

@nishantka nishantka Awaiting requested review from nishantka

Assignees

@prativas22 prativas22

Labels
Ready for Stage verified PR has been E2E tested by a reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@bandana147 @narcis-radu @prativas22 @salonijain3 @mokimo @sharmrj @sonawanesnehal3 @skumar09 @spadmasa