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

remove the metadata filter in the sandbox #63

Merged

Conversation

moadqassem
Copy link
Member

@moadqassem moadqassem commented Mar 16, 2021

Description

Using a custom endpoint instead of using the default endpoint that hegel adjusts

Why is this needed

The reason behind that is, while using the sandbox in combination with tinkerbell's example workflows. The functions.sh will fail due to lack of information in the retrieved metadata from hegel. The default endpoint filters out all the needed metadata such as plan_slug. This PR removes that filtration criteria.

Also I am not sure, I think we are safe to provide these info(the full hardware spec) to the worker while using the Sandbox setup, as this is mainly used as an example setup not as a production one.

Fixes: #64

How Has This Been Tested?

Yes, it was tested locally by setting the env var in the docker-compose.yml file and batch it in the sandbox setup.

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

Signed-off-by: Moath Qasim <moad.qassem@gmail.com>

Signed-off-by: Moath Qasim <moad.qassem@gmail.com>
Copy link
Contributor

@gauravgahlot gauravgahlot left a comment

Choose a reason for hiding this comment

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

Adding a reference to how CUSTOM_ENDPOINTS are used in Hegel - main.go/#L229.

@gianarb gianarb requested review from mmlb and removed request for gianarb March 18, 2021 14:34
@gianarb
Copy link
Contributor

gianarb commented Mar 18, 2021

I have asked @mmlb about this because I am not really sure what Hegel does :)

@mmlb
Copy link
Contributor

mmlb commented Mar 18, 2021

Yep lgtm

@gianarb gianarb added the ready-to-merge Signal to Mergify to merge the PR. label Mar 18, 2021
@mergify mergify bot merged commit 6a954b6 into tinkerbell:master Mar 18, 2021
@mmlb
Copy link
Contributor

mmlb commented Mar 18, 2021

Although maybe the workflows should be updated to hegel's default format? @gianarb should we deprecate workflows in favor of the ones from hub?

@mfranczy
Copy link

@gianarb @mmlb seems this breaks tinkerbell actions, for instance rootio expects that metadata output would be scoped to metadata.instance https://github.com/tinkerbell/hub/blob/main/actions/rootio/v1/pkg/types.go/metadata.go#L14, it doesn't expect the whole metadata object to be returned. How do you want to tackle this? I suggest that either the PR should be reverted and appropriate documentation should be added to old(?) workflows to change CUSTOM_ENDPOINTS or the actions should extract only the metadata.instance part from the response and unmarshal then. What do you think?

/cc @moadqassem

@moadqassem moadqassem deleted the add-custom-endpoints-as-an-env-var branch March 23, 2021 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hegel filters out important information for running TB workflows
5 participants