Skip to content

Add variable to specify desired AMI architecture #141

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeohist
Copy link
Contributor

@jeohist jeohist commented Apr 4, 2025

Description of the change

  • Add var.ami_architecture to allow users to specify their desired AMI architecture.
  • Make data.aws_ami.this conditional on whether var.ami_id is specified. If it is specified, it's not necessary to lookup the AMI ID.
  • Updated the example / test case to test with the var.ami_architecture variable.

Resolves #139

Type of change

  • New feature (non-breaking change that adds functionality);

Checklists

Development

  • All necessary variables have been defined, with defaults if applicable;
  • The code is formatted properly;

Code review

  • The module version is bumped accordingly;
  • Spacelift tests are passing;
  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached;
  • This pull request is no longer marked as "draft";
  • Reviewers have been assigned;
  • Changes have been reviewed by at least one other engineer;

@jeohist jeohist requested a review from a team as a code owner April 4, 2025 09:34
@jeohist jeohist force-pushed the ami_architecture_variable branch from ddde9f3 to 046db0d Compare April 4, 2025 09:36
sephriot
sephriot previously approved these changes Apr 7, 2025
Copy link
Contributor

@sephriot sephriot left a comment

Choose a reason for hiding this comment

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

LGTM

@jeohist jeohist force-pushed the ami_architecture_variable branch 3 times, most recently from 3335e7d to 10e8233 Compare April 14, 2025 10:52
@jeohist jeohist requested a review from a team as a code owner April 14, 2025 10:52
peterdeme
peterdeme previously approved these changes Apr 14, 2025
@peterdeme
Copy link
Contributor

hey, there seems to be an issue. did you have the chance to test it yourself?

image

@peterdeme peterdeme self-requested a review April 14, 2025 10:59
@jeohist jeohist force-pushed the ami_architecture_variable branch from 10e8233 to 80e7900 Compare April 14, 2025 14:22
@jeohist
Copy link
Contributor Author

jeohist commented Apr 14, 2025

@peterdeme Yeah, mixed up the logic. I've updated it to be the same on both sides.

@jeohist jeohist requested a review from sephriot May 3, 2025 21:08
* Add `var.ami_architecture` to allow users to specify their desired AMI architecture.
* Make `data.aws_ami.this` conditional on whether `var.ami_id` is specified. If it is specified, it's not necessary to lookup the AMI ID.
* Updated the example / test case to test with the `var.ami_architecture` variable.

Resolves <spacelift-io#139>
@jeohist jeohist force-pushed the ami_architecture_variable branch from 80e7900 to 50e6a20 Compare May 12, 2025 15:15
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.

Architecture not taken into account for automatic AMI ID selection
3 participants