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

Refactor Staging/Production Alternate Host Implementation #655

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

ronilan
Copy link
Contributor

@ronilan ronilan commented May 19, 2022

Overview

This pull request refactors the implementation of production/staging hosts (see: #533). It includes minor feature changes and is backwards compatible with any previously valid configurations.

This pull request comes "on top" of (i.e. includes changes from) #652 (which in turn comes "on top" of #651, which is "on top" of #648, #649, #650). Work is in preparation for resolving issue #654.

Change

Features

  • Added development_host option. Becomes default option for publish unpublish when present.
  • Changed behavior when alternate hosts are defined. Now production_host acts as alias to host. Defining staging_host or development_host is enough to default publish and unpublish away from production.
  • An invalid s3_host option does not result in error and is instead silently ignored.

Bug Fix #653

  • When a chain of commands that includes publish or unpublish and host is not specifically set via command line or environment variable, all commands in the chain default away from production.

Code

  • Moved logic regarding host selection of host to versioning. This is where all user defined values from package.json are transformed into command options.

Tests

  • Tests of feature where moved from run.test.js to versioning.test.js.
  • Increased test coverage for various options of using the hosts.
  • All tests pass.

ronilan added 13 commits May 4, 2022 12:37
- Updated abi_crosswalk.json to include node 18 (current).
- Added test coverage for cases of unsupported/unknown targets.
- Modified function signature. Instead of augmenting an object (side effect) the function now gets an options object and returns a configuration object.
- Cleaned up of code auto used to extract bucket and region from s3 url.
- Added comments.
- Increased test coverage.
- All tests pass.
- Modified versioning to add bucket name to hosted_path when s3ForcePathStyle is true.
- Modified s3_setup to remove bucket name from prefix when s3ForcePathStyle is true.
- Added test coverage for s3ForcePathStyle cases.
- mocking setup can be more easily used by automated tests and/or manually by developer.
- mock modules moved to lib/mock.
- mock modules are self contained and have a signature similar to that of other modules in the package.
- simplified http mock to remove no longer needed interface.
- modified http mock to support path style bucket access.
- set http mock to work with default npg-mock-bucket bucket.
- separated s3 tests from build tests.
- refactored fetch test and proxy-bcrypt test to work with refactored mock setup.
- set hosts of test apps to point to mock bucket.
- added app1.1 - identical to app1 but using production and staging binary host option.
- added app1.2 - identical to app1 but using explicit host, region, bucket options.
- Added a GitHub Actions workflow that runs whenever there is a push to the repo.
- Workflow includes two jobs:
    - A matrix job of node versions (10, 12, 14, 16, 18) and operating systems (Linux (ubuntu), Mac and Windows (2019 Enterprise)) that runs all tests against mock and then runs s3 tests against a bucket (located at us-east-1-bucket) specified as a repo secret.
    - A matrix job of and NW.js versions (0.64.0, 0.50.2) and node versions (10, 12, ,14, 16) that runs the NW.js test script.
- Modified `scripts/test-node-webkit.sh` so that it can now accept an NW.js version as input. This allows running the script in a GitHub Actions matrix.
- Modified `test/run.util.js` so that it does not set `--msvs_version=2015` when running in GitHub Actions. This is required because current GitHub Actions runner do not support VS Studio 2015.
- Added npm script command `test:s3` to `package.json`  that runs only the s3 tests. This is required because invoking `npx tape test/s3.test.js` on windows does not work as expected.
- Modified `test/proxy-bcrypt.test.js`. Removed uneeded CI conditionals and modified download directory setup/cleanup. Latter was required due to concurrency issues with running tests on Mac, resulting in uncatchable errors during directory removal originating from `rimraf`.
- Moved logic regarding host selection to versioning where all user defined values from package.json are transformed into command options.
- Moved testing of feature from `run.test.js` to `versioning.test.js`.
- Added `development_host` option. Becomes default option for `publish` `unpublish` when present.
- Changed behavior when alternate hosts are defined. Now `production_host` acts as alias to host. Defining `staging_host` or `development_host` is enough to default `publish` and `unpublish` away from production.
- When a chain of commands that includes `publish` or `unpublish`, when host not specifically set via command line or environment variable, ALL commands in the chain default away from production.
- An invalid `s3_host` option does not result in error and is instead silently ignored.
- Change is backwards compatible with previously valid configurations.
(uri) => {
const bucket = 'npg-mock-bucket';
const mockDir = uri.indexOf(bucket) === -1 ? `${basePath}/${bucket}` : basePath;
const filepath = path.join(mockDir, uri.replace('%2B', '+'));

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding High

This replaces only the first occurrence of '%2B'.
function http_mock() {
log.warn('mocking http requests to s3');

const baseHostname = 's3.us-east-1.amazonaws.com';

Check failure

Code scanning / CodeQL

Incomplete regular expression for hostnames High

This string, which is used as a regular expression
here
, has an unescaped '.' before 'amazonaws.com', so it might match more hosts than expected.
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.

Chaining publish and unpublish Commands Changes Their Default Behavior
1 participant