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

remote-config: rename FEATURES test to ASM_FEATURES #565

Merged
merged 15 commits into from
Oct 27, 2022

Conversation

ameske
Copy link
Contributor

@ameske ameske commented Oct 12, 2022

The product FEATURES was renamed to ASM_FEATURES prior to going prod, so this just updates all references of the old name so that new tracers can test against the product that will go live.

Closes: #558
Closes: #559

Description

Please include a short summary of the change

Check list

Your PR is not ready to be reviewed? Please save it as a draft 🙏

  • Follows the style guidelines of this project (See how to easily lint the code)
  • CI is passing
  • There is at least one approval from code owners

Yes to all? Feel free to merge it whenever you want ❤️

@cbeauchesne
Copy link
Collaborator

cbeauchesne commented Oct 13, 2022

If i'm not wrong, these scnearios are not in the CI, is it expected ?

@simon-id
Copy link
Member

simon-id commented Oct 13, 2022

It looks like the file scenarios/remote_config/rc_expected_requests.json didn't get updated by the scenario generator script ? it still has the old FEATURES ? I don't know if this file is used tbh ?

"path": "datadog/2/FEATURES/features1/config",

run.sh Outdated Show resolved Hide resolved
run.sh Outdated Show resolved Hide resolved
run.sh Outdated Show resolved Hide resolved
run.sh Outdated Show resolved Hide resolved
@ameske
Copy link
Contributor Author

ameske commented Oct 13, 2022

It looks like the file scenarios/remote_config/rc_expected_requests.json didn't get updated by the scenario generator script ? it still has the old FEATURES ? I don't know if this file is used tbh ?

It looks like it is not used, an artifact from when we didn't have multiple test scenarios. I'll make another PR to remove it.

@ameske
Copy link
Contributor Author

ameske commented Oct 13, 2022

If i'm not wrong, these scnearios are in the CI, is it expected ?

@cbeauchesne - We don't want disabling APPSEC to cause issues with other tests but I think everything is fine since we isolated the remote config tests to independent executions, if I'm understanding "scenarios" correctly.

@cbeauchesne
Copy link
Collaborator

If i'm not wrong, these scnearios are in the CI, is it expected ?

@cbeauchesne - We don't want disabling APPSEC to cause issues with other tests but I think everything is fine since we isolated the remote config tests to independent executions, if I'm understanding "scenarios" correctly.

Scenario runs are totally isolated (it's their purpose :) )

My point is that those two scenario are never executed in the system tests ci, meaning that we can break them anyday without knowing it. Can you add them in .github/workflow/ci.yml ?

@simon-id
Copy link
Member

@cbeauchesne

If i'm not wrong, these scnearios are in the CI, is it expected ?

I think you forgot a "not" 😄

@cbeauchesne
Copy link
Collaborator

I think you forgot a "not" 😄

indeed

@ameske
Copy link
Contributor Author

ameske commented Oct 14, 2022

Can definitely do this, but might require some more discussion. We have a single "scenario" but multiple flavors of it. (Basically one for each product, and then for each product the "cached" and "no cached" version) We can't run multiple flavors together because of the stateful nature of remote config.

Thus to test everything we'd need this block for all possible "flavors":

 - name: Run REMOTE_CONFIG_MOCKED_BACKEND_<PRODUCT>_<FLAVOR> scenario
   run: ./run.sh REMOTE_CONFIG_MOCKED_BACKEND_<PRODUCT>_<FLAVOR>
   env:
       DD_API_KEY: ${{ secrets.DD_API_KEY }}

Does that sound right/ok to you? If so I'll draw this up in another PR.

@Julio-Guerra
Copy link
Contributor

Hello,
Any chance we can move forward on this so that the ASM libs can rely on those tests? 🙏

@cbeauchesne
Copy link
Collaborator

cbeauchesne commented Oct 24, 2022

@ameske how many product/flavor do you have ?

@ameske ameske force-pushed the kyle.ames/features-to-asm-features branch from f6b2e90 to 0219948 Compare October 24, 2022 13:37
@ameske
Copy link
Contributor Author

ameske commented Oct 24, 2022

I just cleaned up the merge conflict, assuming CI/CD passes here let's go ahead and merge this so that discussion around adding the scenarios to the CI doesn't slow down progress. I'll open a PR with the naive route of registering scenarios with CI/CD and we can take a look and see if there's a better way.

@ameske
Copy link
Contributor Author

ameske commented Oct 24, 2022

Trying to help move this along, but I'm not sure what's going on with all the failures at the moment. I'm trying to reproduce locally but not having any success - the runner is just hanging.

@cbeauchesne
Copy link
Collaborator

I'll have a look

run.sh Outdated Show resolved Hide resolved
run.sh Outdated Show resolved Hide resolved
run.sh Outdated Show resolved Hide resolved
ameske and others added 6 commits October 25, 2022 09:59
The product FEATURES was renamed to ASM_FEATURES prior to going prod,
so this just updates all references of the old name so that new tracers
can test against the product that will go live.
Within tracers, appsec being enabled via the DD_APPSEC_ENABLED
environment variable disables remote configuration. In order to be
able to leverage these tests, we need that environment variable
set to false.
Co-authored-by: simon-id <simon.id@datadoghq.com>
Co-authored-by: simon-id <simon.id@datadoghq.com>
@ameske ameske force-pushed the kyle.ames/features-to-asm-features branch from 43d5b2f to 02cb3eb Compare October 25, 2022 13:59
utils/proxy/forwarder.py Outdated Show resolved Hide resolved
@ameske
Copy link
Contributor Author

ameske commented Oct 25, 2022

Seems the current failures all stem from an issue on main, and not this PR. @cbeauchesne - we're all set with this specific change so we're just needing a system-tests core review now.

Co-authored-by: simon-id <simon.id@datadoghq.com>
@simon-id
Copy link
Member

Status update: the file ./run.sh was somehow chmod'ed to non executable, so I've changed it back

@simon-id
Copy link
Member

Status update: the ruby lib crashes when passed an empty string throught DD_APPSEC_ENABLED I'm working on a solution.

@simon-id simon-id merged commit d9abdd6 into main Oct 27, 2022
@simon-id simon-id deleted the kyle.ames/features-to-asm-features branch October 27, 2022 09:42
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.

Remove the local ASM config in remote config scenarios with ASM_FEATURES Rename FEATURES to ASM_FEATURES
5 participants