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

Support Console secrets in param source variable resolution #707

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

Conversation

medikoo
Copy link
Contributor

@medikoo medikoo commented Jun 26, 2023

On hold, as first, changes to backend API need to be deployed to Console backend prod stage

@medikoo medikoo added the enhancement New feature or request label Jun 26, 2023
@medikoo medikoo self-assigned this Jun 26, 2023
@medikoo medikoo requested a review from Danwakeem June 26, 2023 15:42
@medikoo medikoo force-pushed the 0626-integrate-console-params branch from 7317938 to cf04325 Compare June 26, 2023 15:51
@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Patch coverage: 97.67% and project coverage change: +0.59 🎉

Comparison is base (8282243) 50.31% compared to head (d0cd396) 50.90%.

❗ Current head d0cd396 differs from pull request most recent head cf04325. Consider uploading reports for the commit cf04325 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #707      +/-   ##
==========================================
+ Coverage   50.31%   50.90%   +0.59%     
==========================================
  Files          92       92              
  Lines        2572     2605      +33     
==========================================
+ Hits         1294     1326      +32     
- Misses       1278     1279       +1     
Impacted Files Coverage Δ
lib/resolve-params.js 96.87% <97.67%> (+0.04%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link

@Danwakeem Danwakeem left a comment

Choose a reason for hiding this comment

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

We should use more than just consoleServiceStage and consoleService parameters

Comment on lines +83 to +84
const paramType = resolveConsoleParamType(paramData.path);
if (!paramType) continue;

Choose a reason for hiding this comment

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

It looks like this means that we would only resolve parameters that fall under the expected paths of consoleServiceStage and consoleService but I think we want to be able to resolve parameters that fall outside of that as well.

For example, if a customer sets global parameters (parameter with no path) then we would want that parameter to be resolved as well.

We should be able to just drop the paramType logic and just resolve any parameters returned by the API since the logic deciding which parameters to use or not is already calculated there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, if a customer sets global parameters (parameter with no path) then we would want that parameter to be resolved as well.

That's a very good point @Danwakeem but I think we need to raise it Linear, and discuss with @skierkowski.

Currently there's no concept of global params in Framework, there's just service level and service + stage + region level.

So question here is, how we should treat params with no path set?

Should they be treated same as service level (in such case configuration service level params will override it), or maybe they should take precedence over any configuration service level params?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Danwakeem see the resolution order we agreed on Linear, there's no concept of global params there (at this point)

Choose a reason for hiding this comment

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

Yeah agreed, definitely need @skierkowski input on this 👍 I can follow up there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if those global need to be supported, let's update the spec (agreed resolution order in linear)

Choose a reason for hiding this comment

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

I don't think it is explicitly outlined in linear but it has been discussed in various tickets in linear so it would be good to get more explicit confirmation.

My understanding was that we should treat global parameters as a fallback to consoleService. Again this is not explicitly stated though so would be good to get confirmation 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants