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

fix: only append 'Elastic/Synthetics' to default UA #514

Merged
merged 2 commits into from
Jun 3, 2022

Conversation

andrewvc
Copy link
Contributor

@andrewvc andrewvc commented May 26, 2022

When we originally implemented #232 it appears we didn't implement the third AC, allowing users to override our custom UA string "Elastic/Synthetics" which is appended.

Some users need to fully replace the UA, this allows them to do that. Rather than adding a boolean, this assumes that if you're overriding the UA you want full control, which is more in line with the principle of least surprise.

CC @paulb-elastic @drewpost

PS this is currently a draft because there are no tests, I'll add tests if we agree on this approach.

@andrewvc andrewvc added the bug Something isn't working label May 26, 2022
@andrewvc andrewvc self-assigned this May 26, 2022
@andrewvc andrewvc marked this pull request as draft May 26, 2022 19:52
@apmmachine
Copy link

apmmachine commented May 26, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-06-03T00:23:25.513+0000

  • Duration: 15 min 30 sec

Test stats 🧪

Test Results
Failed 0
Passed 164
Skipped 2
Total 166

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

@andrewvc Should be good to go after we fix the tests.

@vigneshshanmugam vigneshshanmugam changed the title Only append 'Elastic/Synthetics' to default UA fix: only append 'Elastic/Synthetics' to default UA Jun 1, 2022
andrewvc and others added 2 commits June 2, 2022 17:18
When we originally implemented
elastic#232 it appears we didn't
implement the third AC, allowing users to override our custom UA string
"Elastic/Synthetics" which is appended.

Some users need to fully replace the UA, this allows them to do that.
Rather than adding a boolean, this assumes that if you're overriding the
UA you want full control, which is more in line with the principle of
least surprise.
@vigneshshanmugam vigneshshanmugam marked this pull request as ready for review June 3, 2022 00:23
@vigneshshanmugam vigneshshanmugam merged commit f7a5c8f into elastic:main Jun 3, 2022
@andrewvc andrewvc removed their assignment Jun 7, 2022
@justinkambic justinkambic self-assigned this Jun 7, 2022
@justinkambic
Copy link
Contributor

Post-FF testing LGTM:

I set up a project like:

// synthetics.config.js
const config = {
  playwrightOptions: {
    userAgent: "DO NOT SAY ELASTIC",
  },
};

module.exports = config;
// test.journey.ts
const { journey, step, expect } = require('@elastic/synthetics');

journey(
  { name: 'Test', tags: ['browse'] },
  ({ page, params }) => {

    step('go to my untrusted HTTPS endpoint', async () => {
      await page.goto('https://www.whatismybrowser.com/');
      await new Promise(r => setTimeout(r, 50000));
    });
  }
);

When running the journey, I got the following output:

image

@shahzad31
Copy link
Contributor

POST FF Testing looks good !!

@andrewvc andrewvc deleted the only-append-ua-default branch June 7, 2022 18:47
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.

5 participants