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

proposal for env var sourcing [EE-2542] #94

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

deviantony
Copy link
Member

This PR proposes a way to load environment variables passed to the script with the new feature available through https://portainer.atlassian.net/browse/EE-2436

This feature introduce a new parameter for the script: a single string containing multiple environment variables separated by a comma (e.g. VAR1,VAR2).

This is a proposal and feels a bit hacky, suggestions are welcome. Maybe we could generate a YAML file with the environment content and use --from-file instead of --from-literal?

I wasn't sure which file needed to be updated so I updated both.

@deviantony
Copy link
Member Author

In order to test it:

I'm thinking we need to open another PR as an intermediary PR (based on this PR), e.g. target different files (suffixed by -dev or something), upload the files in the appropriate locations (download.portainer.io), update the Portainer code to reference this -dev suffixed script, test it.

After validation, merge this PR and remove the intermediary PR changes.

Thoughts @ssbkang @samdulam @yi-portainer?

@samdulam
Copy link
Collaborator

In order to test it:

I'm thinking we need to open another PR as an intermediary PR (based on this PR), e.g. target different files (suffixed by -dev or something), upload the files in the appropriate locations (download.portainer.io), update the Portainer code to reference this -dev suffixed script, test it.

After validation, merge this PR and remove the intermediary PR changes.

Thoughts @ssbkang @samdulam @yi-portainer?

We only need to change the file location in the script to the manifest file in this branch. Once tested change it back before merge?

@deviantony
Copy link
Member Author

deviantony commented Feb 14, 2022

TODO:

  • Revert curl https://raw.githubusercontent.com/portainer/k8s/fc0044800a8cb4be6c19f88aac1c696cef7ba916/deploy/manifests/agent/portainer-ce211-agent-edge-k8s.yaml to curl -L https://portainer.github.io/k8s/deploy/manifests/agent/portainer-ce211-agent-edge-k8s.yaml in this PR

  • Revert curl https://raw.githubusercontent.com/portainer/k8s/fc0044800a8cb4be6c19f88aac1c696cef7ba916/deploy/manifests/agent/portainer-ce211-edge-agent-setup.sh to https://downloads.portainer.io/portainer-ce${agentVersion}-edge-agent-setup.sh in feat(endpoint): add an input to source env vars [EE-2436] portainer#6517 and https://github.com/portainer/portainer-ee/pull/1190

@deviantony
Copy link
Member Author

FYI, running the script gave me the following output @ssbkang :

curl https://raw.githubusercontent.com/portainer/k8s/fc0044800a8cb4be6c19f88aac1c696cef7ba916/deploy/manifests/agent/portainer-ce211-edge-agent-setup.sh | bash -s -- 1ddb8ea3-ab2a-4668-a42a-078d2a82664e aHR0cHM6Ly8yMDYuMTg5LjkyLjIzMjo5NDQzfDIwNi4xODkuOTIuMjMyOjgwMDB8MTg6Zjg6Nzk6NjE6OTU6MTI6MGI6NzA6YjM6MTU6YjQ6ZGI6NjI6MDc6Njk6Zjl8Mw 1 HOST,MACHINE_ID
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  3544  100  3544    0     0   9844      0 --:--:-- --:--:-- --:--:--  9871
Downloading agent manifest...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2457  100  2457    0     0   7875      0 --:--:-- --:--:-- --:--:--  7875
Creating Portainer namespace...
namespace/portainer created
Creating agent configuration...
main: line 78: configmap/portainer-agent-edge: No such file or directory
Creating agent secret...
secret/portainer-agent-edge-key created
Deploying agent...
Warning: resource namespaces/portainer is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
namespace/portainer configured
serviceaccount/portainer-sa-clusteradmin created
clusterrolebinding.rbac.authorization.k8s.io/portainer-crb-clusteradmin created
service/portainer-agent created
deployment.apps/portainer-agent created

@deviantony
Copy link
Member Author

Note that this is not required anymore for https://portainer.atlassian.net/browse/EE-2436 as we're planning to support Docker and Swarm to start with.

We should keep it open as a reference for https://portainer.atlassian.net/browse/EE-2542 though.

Copy link

@WaysonWei WaysonWei left a comment

Choose a reason for hiding this comment

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

LGTM

@deviantony
Copy link
Member Author

deviantony commented Feb 18, 2022

There is no need to merge this for the EE-2.12 release.

@chiptus
Copy link
Contributor

chiptus commented Mar 2, 2022

I wasn't sure which file needed to be updated so I updated both.

I am also not sure yet why we have a whole bunch of files ce11... instead of using git tags for that.

it looks good, just need to create files for the correct CE/EE version

@chiptus chiptus changed the title proposal for env var sourcing proposal for env var sourcing [EE-2542] Mar 3, 2022
@chiptus chiptus force-pushed the feat/EE-2436/source-env-vars branch from 726a656 to 2c984d4 Compare April 12, 2022 07:59
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.

5 participants