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

Allow to provide custom LHCI config #22

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

szimek
Copy link

@szimek szimek commented Oct 25, 2021

This PR introduces changes that allow one to provide a partial custom LHCI config that gets merged with the default one provided in entrypoint.sh file and fixes #20.

It's a bit of a wip, but I wanted to get some feedback first. Things that are missing:

  • make the path configurable (the default is set to .lighthouserc.yml)
  • update README.md file
  • update Dockerfile to use the updated base image, once it's published

It's also possible to overwrite some of the other inputs (e.g. LHCI scores) with this config, which might be a bit confusing.

I tested it with act and it works. I also pushed the new base Docker image that includes yq to szimek/lighthouse-ci-action:1.1.0.

@ghost ghost added the cla-needed label Oct 25, 2021
@ghost ghost removed the cla-needed label Oct 25, 2021
@charlespwd charlespwd self-assigned this Oct 25, 2021
entrypoint.sh Outdated
@@ -195,6 +196,11 @@ ci:
aggregationMethod: median-run
EOF

# Merge custom Lighthouse CI config, if provided
if [[ -f ".github/lighthouserc-custom.yml" ]]; then
yq eval-all 'select(fileIndex == 0) * select(fileIndex == 1)' lighthouserc.yml .github/lighthouserc-custom.yml -i
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'd just use .lighthouserc.yml as default since that's the default for LHCI. Probably better for us to handle this complexity. Would make our code slightly more complicated, but we're taking that complexity away from the devs.

Maybe something like:

default_config="$(mktemp XXXXX.yml)"
cat <<- EOF > $default_config
  ...
EOF

if [[ -f ".lighthouserc.yml" ]]; then
  yq eval-all '...' $default_config .lighthouserc.yml > .lighthouserc.yml.tmp
  mv .lighthouserc.yml.tmp .lighthouserc.yml
else
  mv $default_config .lighthouserc.yml
else

Copy link
Author

Choose a reason for hiding this comment

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

Much better!

I'd just make sure that it's clearly explained in the README that it doesn't have to be the complete config.

@charlespwd
Copy link
Collaborator

It's also possible to overwrite some of the other inputs (e.g. LHCI scores) with this config, which might be a bit confusing.

I think that's OK. If you're overwriting them, then it's really on you :P.

I think we can fix that with some kind of "Advanced Usage" docs that shows the default configuration and how you can change that.

The default path to the custom LHCI config is .lighthouserc.yml, but it can be customized.
The custom config will be merged with the default one.
@szimek
Copy link
Author

szimek commented Nov 7, 2021

@charlespwd I've just pushed an updated version.

One thing that I found a bit confusing is the default value for inputs in action.yml - I thought that when the default value is set, it will cause the respective environment variable to be set up with this default value. It looks like it's not the case, though.

Anyway, while working on it, I realized that one can overwrite any LHCI configuration setting with environment variables, so it's already possible to modify the default upload config with:

jobs:
  lhci:
    name: Lighthouse
    runs-on: ubuntu-latest
    steps:
       - name: Lighthouse
         ...
         env:
           LHCI_UPLOAD__TARGET: lhci
           LHCI_UPLOAD__SERVER_BASE_URL: ...
           LHCI_UPLOAD__TOKEN: ...
           LHCI_UPLOAD__BASIC_AUTH__USERNAME: ...
           LHCI_UPLOAD__BASIC_AUTH__PASSWORD: ...

Maybe it's not very convenient, but possible.

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.

Allow to upload results to LH CI server
2 participants