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

Issue 652: Add instance placement scriptlet extension #780

Merged
merged 6 commits into from May 1, 2024

Conversation

christina-zh
Copy link

PR for #652
We were unable to validate our changes could you please validate for us?

@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Apr 22, 2024
@stgraber
Copy link
Member

Hey, just wanted to let you know that I'm not ignoring this one, I'm just traveling and don't have easy access to test clusters to validate the logic, at worst I'll be able to do it on Monday.

@christina-zh
Copy link
Author

Hey, just wanted to let you know that I'm not ignoring this one, I'm just traveling and don't have easy access to test clusters to validate the logic, at worst I'll be able to do it on Monday.

Ok sounds good, thank you for your time!

Signed-off-by: Yueyuanmei Zhang <yymzhang@cs.utexas.edu>
@stgraber
Copy link
Member

Looking into this one now:

  • Did a quick rebase due to conflicts introduced by changes we merged since
  • Ran gofmt -s -w ./ to fix some code style issues
  • Ran make static-analysis to check the rest and fixed the issues it identified

I'll take a closer look at the logic in both functions and do a bit of testing on this next.

Signed-off-by: Yueyuanmei Zhang <yymzhang@cs.utexas.edu>
@stgraber
Copy link
Member

There seems to be some issues with the logic, it effectively requires the arguments instead of having them be optional.

Error: Failed instance creation: Failed instance placement scriptlet: Failed to run: get_instances: missing argument for project
Error: Failed instance creation: Failed instance placement scriptlet: Failed to run: get_cluster_members: missing argument for group

@stgraber
Copy link
Member

I'll look at fixing those, shouldn't be too hard, I think I'll also add a 3rd function to the list as it'd be handy for one of my own clusters.

@christina-zh
Copy link
Author

ok, thank you so much!

Yueyuanmei Zhang added 4 commits April 30, 2024 19:03
Signed-off-by: Yueyuanmei Zhang <yymzhang@cs.utexas.edu>
Signed-off-by: Yueyuanmei Zhang <yymzhang@cs.utexas.edu>
Signed-off-by: Yueyuanmei Zhang <yymzhang@cs.utexas.edu>
Signed-off-by: Yueyuanmei Zhang <yymzhang@cs.utexas.edu>
@stgraber
Copy link
Member

Right, so what I had to do was:

@stgraber
Copy link
Member

Ended up filing #811 for the extra bit

@stgraber stgraber merged commit 7912fa9 into lxc:main May 1, 2024
25 checks passed
@aanderse
Copy link

aanderse commented May 1, 2024

thank you so much for working on this @christina-zh and @stgraber!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating
Development

Successfully merging this pull request may close these issues.

None yet

3 participants