Feature/adding ci git actions #165

DanBezalelpx · 2023-06-13T09:07:35Z

No description provided.

ori-gold-px

Lots of demo site things here can be removed and simplified -
(1) No need for all the templates, build/run scripts, and all the different configuration files. I say simplify this as much as possible!
(2) Remove all identifiers from the demo app (app IDs, tokens, etc.) - these should be included as repo secrets in GH, but never ever hard-coded in the repo.

ori-gold-px · 2023-06-26T16:35:00Z

demo-site/servers/nodejs/px_config.json

@@ -0,0 +1,87 @@
+{


Lots of these files you don't need - for instance, there's no need to have a shared_config.json and a px_config.json and a config.json and a config.inc.json. Ultimately, all these are to generate one px_config.json file that's used for the sample site.

@DanBezalelpx Still all 4 of these files - I don't think you need all of them.

removed most of them (only the unnecessary with safe remove

demo-site/servers/nodejs/config.json

demo-site/templates/origin/origin_app.js

demo-site/templates/test_endpoints/test_endpoints_app.js

demo-site/scripts/build_docker.sh

demo-site/scripts/run_docker.sh

demo-site/templates/origin/Dockerfile

demo-site/utils/cdn_deploy_tool_utils.js

ci_files/build_cluster.sh

.github/workflows/E2E_CI.yaml

ori-gold-px · 2023-07-14T15:48:29Z

.github/workflows/E2E_CI.yaml

+  extract_metadata:
+    runs-on: ubuntu-latest
+    name: Extract supported_features
+    outputs:
+      supported-features: ${{ steps.supported-features.outputs.value }}
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v3
+      - name: Setup Node.js
+        uses: actions/setup-node@v3
+        with:
+          node-version: '18.x'
+      - name: extract supported features
+        id: supported-features
+        run:  echo "value=$(node -p -e "require('./px_metadata.json').supported_features?.join(' or ') || ''")" >> "$GITHUB_OUTPUT"


Why is this a separate job? Do we need it for something else?

We made it as a dependency in all of our new ci cd + i don't think it should be part of the CI action

ori-gold-px · 2023-07-14T15:50:44Z

.github/workflows/E2E_CI.yaml

+          docker pull gcr.io/px-docker-repo/connecteam/mock-collector:1.0.2 && \
+          docker tag gcr.io/px-docker-repo/connecteam/mock-collector:1.0.2 localhost:5001/mock-collector:1.0.2 && \
+          docker push localhost:5001/mock-collector:1.0.2 && \


Is there a way to put the mock collector version in a variable or something? I'm thinking when we'll want to update it, finding it in all these places in the yaml will be annoying. Maybe we can add it to the px_metadata.json as well to have the versions of mock collector and tests so that we only need to update them in one place?

We're currently thinking of a way to include it as part of the tests set (for each tests version, we'll have a matching mock-collector version)
For now - we made it that way on every CI/CD and we're planning to address this issue across all ci/cd, I think that we can deploy the CI this way, and address it all together once we'll have a stable version.
@guyeisenbach FYI

ori-gold-px · 2023-07-14T15:50:57Z

.github/workflows/E2E_CI.yaml

+          docker pull gcr.io/px-docker-repo/connecteam/enforcer-specs-tests:1.1.0 && \
+          docker tag gcr.io/px-docker-repo/connecteam/enforcer-specs-tests:1.1.0 localhost:5001/enforcer-spec-tests:1.1.0 && \
+          docker push localhost:5001/enforcer-spec-tests:1.1.0 && \


See above comment for mock collector

ori-gold-px · 2023-07-14T15:53:58Z

demo-site/servers/nodejs/px_config.json

@@ -0,0 +1,87 @@
+{


@DanBezalelpx Still all 4 of these files - I don't think you need all of them.

demo-site/scripts/create_px_configs.js

demo-site/scripts/create_static_files.js

ori-gold-px · 2023-07-14T16:06:59Z

demo-site/servers/nodejs/app.js

+        for (const [name, value] of Object.entries(req.headers)) {
+            res.setHeader(name, value);
+        }


This was added so that the px-compromised-credentials header on the request would be transferred over to the response. It's not enough to transfer just that header, though, because it could change based on the enforcer configuration... but transferring ALL the headers to the response could be problematic? Ideally we would transfer only the px_compromised_credentials_header config value.

@DanBezalelpx I would modify this to be only the px-compromised-credentials header.

demo-site/servers/nodejs/app.js

px_metadata.json

ori-gold-px · 2023-08-03T22:13:02Z

demo-site/shared_config.json

@@ -0,0 +1,83 @@
+{


I think having this is unnecessary - or at least it's unnecessary to have the "enforcer_config" property here since the px_config.json file has the same info. I'd say choose one place to put this information - one source of truth.

ori-gold-px · 2023-08-03T22:13:45Z

demo-site/scripts/create_static_files.js

@@ -0,0 +1,105 @@
+//region imports
+const fs = require('fs');


This may not be necessary but it can stay because we still have to generate the static files based on the app_id and first_party configs so it's okay for now.

ori-gold-px · 2023-08-03T22:14:26Z

demo-site/utils/constants.js

+const CDN_DEPLOY_TOOL_CONFIG_FILE_NAME = "cliConfig.json";
+const CDN_DEPLOY_TOOL_RUN_CLI_COMMAND = "npm run cli";
+const CDN_DEPLOY_TOOL_BUILD_COMMAND = "npm run build";
+
+const PX_METADATA_FILE_NAME = "px_metadata.json";
+
+const TEST_APP_CREDENTIALS_ENDPOINT = "/test-app-credentials";
+const SUPPORTED_FEATURES_ENDPOINT = "/supported-features";
+const CONFIG_ENDPOINT = "/config";


Re-evaluate if all these constants are necessary? Some of them (especially CDN deploy tool ones) may not be.

ori-gold-px requested changes Jun 26, 2023

View reviewed changes

DanBezalelpx force-pushed the feature/adding_ci_git_actions branch from 7f0c342 to a4e586b Compare June 26, 2023 18:22

PerimeterX deleted a comment from ori-gold-px Jun 26, 2023

ci for express

072e114

DanBezalelpx force-pushed the feature/adding_ci_git_actions branch from a4e586b to 072e114 Compare June 26, 2023 18:35

DanBezalelpx added 2 commits June 26, 2023 21:37

ci for express

2a92e69

removed unnececery files

23733df

guyeisenbach requested changes Jun 28, 2023

View reviewed changes

DanBezalelpx added 2 commits July 3, 2023 19:24

fix comments after pr

418aa8c

fix comments after pr

386ae63

guyeisenbach approved these changes Jul 3, 2023

View reviewed changes

ori-gold-px reviewed Jul 14, 2023

View reviewed changes

DanBezalelpx added 5 commits July 19, 2023 15:02

fix comments after pr

3769ac5

fix comments after pr

1cc6783

fix comments after pr

99f494f

fix comments after pr

900e4f7

fix comments after pr

b30648b

ori-gold-px reviewed Aug 3, 2023

View reviewed changes

DanBezalelpx added 3 commits August 6, 2023 18:22

fix comments after pr

18f5a9f

fix comments after pr

918398c

fix comments after pr

6aea47c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/adding ci git actions #165

Feature/adding ci git actions #165

DanBezalelpx commented Jun 13, 2023

ori-gold-px left a comment

ori-gold-px Jun 26, 2023

ori-gold-px Jul 14, 2023

DanBezalelpx Jul 26, 2023

ori-gold-px Jul 14, 2023

DanBezalelpx Jul 19, 2023

ori-gold-px Jul 14, 2023

DanBezalelpx Jul 19, 2023

ori-gold-px Jul 14, 2023

DanBezalelpx Jul 19, 2023

ori-gold-px Jul 14, 2023

ori-gold-px Jul 14, 2023

ori-gold-px Aug 3, 2023

ori-gold-px Aug 3, 2023

ori-gold-px Aug 3, 2023

ori-gold-px Aug 3, 2023

Feature/adding ci git actions #165

Are you sure you want to change the base?

Feature/adding ci git actions #165

Conversation

DanBezalelpx commented Jun 13, 2023

ori-gold-px left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment