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

Set system-owned envs after user-provided envs #1701

Merged
merged 4 commits into from
May 27, 2021

Conversation

kevin-bates
Copy link
Member

@kevin-bates kevin-bates commented May 26, 2021

The changes introduced in #1668 to indicate the runtime name within an operation failed when an operation referenced ELYRA_RUNTIME_ENV on KFP and Local runtimes (Airflow was fine). This corrects those issues and addresses test cases to better handle that scenario - since that is the use case that is being addressed.

How was this pull request tested?

Extended the tests to include cases where the system-owned variables are set in the operation to ensure the system values are properly reflected. Ran a test against KFP since the unit tests test this functionality at a lower level for the Kfp and Airflow processors. (The local test can address this since it includes an end-to-end operation.)

Fixes: #1696

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@elyra-bot
Copy link

elyra-bot bot commented May 26, 2021

Thanks for making a pull request to Elyra!

To try out this branch on binder, follow this link: Binder

@kevin-bates kevin-bates added this to the 2.3.0 milestone May 26, 2021
@lresende
Copy link
Member

Should we allow the user to discover + set these at all?

@kevin-bates
Copy link
Member Author

Should we allow the user to discover + set these at all?

System-owned properties, in general, were brought up on the issue (#1696 (comment)) and we should probably discuss it as something outside of this particular PR.

Generally speaking, I don't think users should be allowed to set system-owned envs. ELYRA_RUNTIME_ENV is a system-owned env that the system controls, yet for use by pipeline authors. It should not be user-settable. However, when we considered accessing a Gateway server from a (non-local) runtime, we were going to expose all of the gateway configuration "knobs" via ELYRA_GATEWAY_ variables. So those could be considered something that should be user-settable. But are they considered system-owned in that case? I think so. So then it really becomes an issue as to how the application would know what is and what is not user-settable. Since we don't have any ELYRA_-prefixed envs that we think should be user-settable at this moment, we could make a statement that ELYRA_-prefixed envs are system-owned and non-user-settable. But, envs make great "knobs" and we might want a user-settable, system-owned ELYRA-prefixed env someday. 😄

So perhaps the answer is: ELYRA_-prefixed envs are system-owned, and, yes, you can set them. However, non-user-settable system-owned envs will NOT honor your value and WILL BE overridden by the server. Because they are truly system-owned. In this case, the best "discovery" mechanism for user's setting system-owned envs will be the documentation.

(We also have some AWS_ variables that should be considered system-owned, despite the fact that they derive their values from runtime-configuration properties (i.e., user-controlled metadata). These should not be settable outside of the values the server pulls from the runtime configuration to set them - nor should they appear anywhere (so non-discoverable).)

Just my two cents.

@ptitzler
Copy link
Member

we should probably discuss it as something outside of this particular PR.

I agree. Created #1704 for follow-up

the best "discovery" mechanism for user's setting system-owned envs will be the documentation.

Created #1703 for follow-up.

@ptitzler
Copy link
Member

Tested local/kfp/airflow and ELYRA_RUNTIME_ENV. Seems to work now as expected.

@lresende lresende added the status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. label May 26, 2021
@lresende lresende removed the status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. label May 26, 2021
Copy link
Member

@ptitzler ptitzler left a comment

Choose a reason for hiding this comment

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

LGTM

@lresende lresende merged commit afb7036 into elyra-ai:master May 27, 2021
@kevin-bates kevin-bates deleted the protect-system-envs branch September 11, 2021 23:33
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.

ELYRA_RUNTIME_ENV variable is no longer set during pipeline execution or export
3 participants