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

Allow env access in Flows Run Script operation #16111

Merged
merged 12 commits into from Jan 4, 2023
Merged

Conversation

licitdev
Copy link
Member

@licitdev licitdev commented Oct 21, 2022

Description

Implements #16108. Allow access to environment variables via data['$env'] or data.$env or process.env in Flows Run Script operation.

Type of Change

  • Bugfix
  • Improvement
  • New Feature
  • Refactor / codestyle updates
  • Other, please describe:

Requirements Checklist

  • New / updated tests are included
  • All tests are passing locally
  • Performed a self-review of the submitted code

If adding a new feature:

@licitdev licitdev marked this pull request as ready for review October 21, 2022 17:35
Copy link
Member

@br41nslug br41nslug left a comment

Choose a reason for hiding this comment

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

Looks like a solid approach to me 😄 I am wondering tho if we should stay on the safe side and block KEY/SECRET (or possibly database credentials) from being "allowed" 🤔

@rijkvanzanten
Copy link
Member

should stay on the safe side and block KEY/SECRET (or possibly database credentials) from being "allowed" 🤔

I don't think that's necessary. That configuration is in the env vars, which is sysadmin only. That's already "you have access to everything" access, so it's a free for all at that point.

@licitdev
Copy link
Member Author

The $env variable is consistent with other variables such as $last, $accountability and $trigger where it can be used within the options.

There are numerous ways to accessed the variables in the run script operation as follows.
Should the global variable be removed?

module.exports = async function (data) {
	// The following logs the same value
	console.log(process.env); // Node.js method
	console.log($env); // Global variable
	console.log({{$env}}); // Parsed options
	console.log(data.$env) // Passed data, or data['$env']
};

@licitdev licitdev marked this pull request as draft October 28, 2022 10:34
@br41nslug
Copy link
Member

Should the global variable be removed?

Yeah i think that one might be a bit overkill, none of the others are directly accessible as global variables right?

@licitdev
Copy link
Member Author

@br41nslug That has been added that in #16180 🤔

@br41nslug
Copy link
Member

Ah i did not know that 😓 never mind then

@licitdev licitdev marked this pull request as ready for review October 28, 2022 16:07
@licitdev
Copy link
Member Author

@br41nslug The global variable in the run script operation is indeed overkill and hard to document. Removed! 😄

Copy link
Member

@nickrum nickrum left a comment

Choose a reason for hiding this comment

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

I'm wondering if we want to remove the exec-operation-specific handling of env vars completely to make it consistent with the other operations 🤔

api/src/env.ts Outdated Show resolved Hide resolved
@licitdev licitdev requested a review from nickrum October 29, 2022 03:09
@br41nslug
Copy link
Member

Have change requests been resolved to merge this one @licitdev ?

@licitdev
Copy link
Member Author

licitdev commented Jan 4, 2023

@br41nslug Yes they were.

@br41nslug br41nslug merged commit d01c4cb into main Jan 4, 2023
@br41nslug br41nslug deleted the feat/flows-allowed-env branch January 4, 2023 09:39
@rijkvanzanten rijkvanzanten added this to the Next Release milestone Jan 11, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants