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

Copy over config file for consumption in container #65

Closed
wants to merge 3 commits into from

Conversation

christian-bromann
Copy link
Contributor

Proposed changes

I feel like it can become handy to copy over the config file to the runner for consumption in order to not pollute the environment with data. I am currently working on a runner for WebdriverIO and it would be nice if user could modify the WDIO config file via the sauce.yaml config.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

work in progress

@@ -85,6 +85,7 @@ func (r *Runner) Run() (int, error) {
fmt.Sprintf("SAUCE_REGION=%s", r.Project.Sauce.Region),
fmt.Sprintf("TEST_TIMEOUT=%d", r.Project.Timeout),
fmt.Sprintf("BROWSER_NAME=%s", r.Suite.Capabilities.BrowserName),
fmt.Sprintf("CONFIG_FILE_PATH=%s", r.Project.ConfigFilePath),
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, I also favor the communication via config files, rather than a bunch of env variables.
What's the information that you think of reading from the config.yaml once you are inside the container?

  1. My concern here is mainly that of "separation of concerns". That config is really meant for saucectl and it should be free to change its definition without concerning itself with anyone else.

I am actually touching a similar subject right now with regards to other configuration (see saucelabs/sauce-puppeteer-runner#23) communicated to the test framework. The idea is that we'll have a separate config which is generated by saucectl, that's meant to be consumed by the test framework itself. These changes will also be accompanied by changes within saucectl.

  1. CONFIG_FILE_PATH is imho a bit generic and prone to clashing with something else in the user's CI environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the information that you think of reading from the config.yaml once you are inside the container?

I think we have two types of configuration, one related to the test runner environment, e.g. capabilities, testrunner image, sauce specific options like region or build. The other type is more test related, e.g. in the WebdriverIO case which framework to run, which reporter to use, timeout settings etc.

My concern here is mainly that of "separation of concerns". That config is really meant for saucectl and it should be free to change its definition without concerning itself with anyone else.

Totally understand. One approach could be to work with different configuration types. This was the reason for the initial design of these yaml files to extend them later and have different configuration types, e.g. network mappings etc. That said, what do you think of having a general saucectl config like this:

apiVersion: v1
kind: orchestration
metadata:
  name: Feature XYZ
  tags:
    - e2e
    - release team
    - other tag
  build: Release $CI_COMMIT_SHORT_SHA
files:
  - tests/example.test.js
capabilities:
  - browserName: Chrome
image:
  base: saucelabs/stt-webdriverio-node
  version: latest
sauce:
  region: us-west-1

and have a second config type defined by every test runner individually that allows for configurations of the framework in use, e.g.

apiVersion: v1
kind: testrunner

framework: mocha
reporter:
  - allure
  - spec
  - junit
services:
  - sauce
# ...

Note the property kind similar to Kubernetes yaml structure. Maybe worth of thinking about a different wording here to not confuse people.

  1. CONFIG_FILE_PATH is imho a bit generic and prone to clashing with something else in the user's CI environment.

Agree. If we decide on above proposal we could think of a fixed path for configs to be placed by saucectl. Generally we might want to prefix all environment variables with SAUCECTL_.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so as well! I was always thinking that we may eventually need a separate configuration that's specific to the framework, otherwise one might pollute the main config with very framework specific settings that are irrelevant most of the time.

SAUCECTL_, SAUCE_ or SLDC_ (short for Souce Labs Dot Com) would make sense!

@@ -25,6 +26,12 @@ type Runner struct {
tmpDir string
}

// FilesToCopy represents data structure to copy over files to target dirs in Docker container
type FilesToCopy struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well keep the struct private.

@dpgraham
Copy link
Contributor

dpgraham commented Mar 5, 2021

Closing this for now, due to inactivity. Feel free to re-open if you want to pick up work on this again.

@dpgraham dpgraham closed this Mar 5, 2021
@alexplischke alexplischke deleted the cb-propagate-config-file branch May 9, 2022 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants