Skip to content

Conversation

alfred-stokespace
Copy link

This required a version bump of dependencies.

Also, added a minor nice-to-have feature of an AMI label prefix that gh workflow can use in order to customize the AMI per workflow if desired.

Testing:

  1. Local testing was completed successfully
  2. Integration testing was also performed and resulted in an instance being created in my account.

Of note:

  1. I added the one and only print statement for this module, let me know if you don't approve of that, I'll remove. Seemed useful if you also like the idea of dynamic ami label. Confirmed that log msg is visible from myshoes output.
  2. I found a bug in this reference impl for the UserData field. API docs state that it must be base64 and that was confirmed in integration testing.

alfred-stokespace and others added 5 commits April 30, 2024 10:42
```
export AWS_ACCESS_KEY_ID=test
alfreds@DESKTOP:/depot/shoes-aws$ export AWS_SECRET_ACCESS_KEY=DOESNOTMATTER
alfreds@DESKTOP:/depot/shoes-aws$ go test .
ok      github.com/whywaita/myshoes-providers/shoes-aws 6.596s
alfreds@DESKTOP:/depot/shoes-aws$ go test -v .
2024/04/30 10:49:03 github.com/testcontainers/testcontainers-go - Connected to docker:
  Server Version: 24.0.7
  API Version: 1.43
  Operating System: Docker Desktop
  Total Memory: 11966 MB
  Resolved Docker Host: unix:///var/run/docker.sock
  Resolved Docker Socket Path: /var/run/docker.sock
  Test SessionID: e4293bc556677a73dab93c8762c6d1d9286f684a42e55acb8856fcedae5ee34b
  Test ProcessID: 40db7acd-b7fc-4147-b57b-6ba2ee746917
2024/04/30 10:49:03 Setting HOSTNAME_EXTERNAL to localhost (to match host-routable address for container)
2024/04/30 10:49:03 🐳 Creating container for image testcontainers/ryuk:0.7.0
2024/04/30 10:49:03 ✅ Container created: 8a674cf56646
2024/04/30 10:49:03 🐳 Starting container: 8a674cf56646
2024/04/30 10:49:03 ✅ Container started: 8a674cf56646
2024/04/30 10:49:03 🚧 Waiting for container id 8a674cf56646 image: testcontainers/ryuk:0.7.0. Waiting for: &{Port:8080/tcp timeout:<nil> PollInterval:100ms}
2024/04/30 10:49:04 🔔 Container is ready: 8a674cf56646
2024/04/30 10:49:04 🐳 Creating container for image localstack/localstack:1.4.0
2024/04/30 10:49:04 ✅ Container created: 0962c1520971
2024/04/30 10:49:04 🐳 Starting container: 0962c1520971
2024/04/30 10:49:04 ✅ Container started: 0962c1520971
2024/04/30 10:49:04 🚧 Waiting for container id 0962c1520971 image: localstack/localstack:1.4.0. Waiting for: &{timeout:0xc0004e2418 Port:4566/tcp Path:/_localstack/health StatusCodeMatcher:0x1897600 ResponseMatcher:0x18cd240 UseTLS:false AllowInsecure:false TLSConfig:<nil> Method:GET Body:<nil> Headers:map[] ResponseHeadersMatcher:0x18cd260 PollInterval:100ms UserInfo: ForceIPv4LocalHost:false}
2024/04/30 10:49:08 🔔 Container is ready: 0962c1520971
=== RUN   Test_createRunnerInstance
--- PASS: Test_createRunnerInstance (0.92s)
=== RUN   Test_deleteRunnerInstance
--- PASS: Test_deleteRunnerInstance (0.56s)
PASS
2024/04/30 10:49:09 🐳 Terminating container: 0962c1520971
2024/04/30 10:49:10 🚫 Container terminated: 0962c1520971
ok      github.com/whywaita/myshoes-providers/shoes-aws 6.700s
```
```
=== RUN   Test_createRunnerInstance
AMI used: ami-test
--- PASS: Test_createRunnerInstance (32.15s)
=== RUN   Test_deleteRunnerInstance
AMI used: ami-02868af3c3df4b3aa
--- PASS: Test_deleteRunnerInstance (4.94s)
PASS
```
dynamic label AMI was tested from within myshoes. You can see the logging message in the output below.
```
2024-04-30T11:49:35.187-0700 [DEBUG] plugin: starting plugin: path=/depot/shoes-aws/shoes-aws args=[/depot/shoes-aws/shoes-aws]
2024-04-30T11:49:35.188-0700 [DEBUG] plugin: plugin started: path=/depot/shoes-aws/shoes-aws pid=33778
2024-04-30T11:49:35.188-0700 [DEBUG] plugin: waiting for RPC address: path=/depot/shoes-aws/shoes-aws
{"@Level":"debug","@message":"plugin address","@timestamp":"2024-04-30T11:49:35.196584-07:00","address":"/tmp/plugin3703107600","network":"unix"}
2024-04-30T11:49:35.196-0700 [DEBUG] plugin: using plugin: version=1
2024-04-30T11:49:35.196-0700 [DEBUG] plugin.shoes-aws: plugin address: address=/tmp/plugin3703107600 network=unix timestamp=2024-04-30T11:49:35.196-0
700
2024-04-30T11:49:35.199-0700 [TRACE] plugin.stdio: waiting for stdio data
2024-04-30T11:49:35.201-0700 [TRACE] plugin.stdio: received data: channel=STDOUT len=32
AMI used: ami-0b214dd7c2834b646
2024-04-30T11:49:35.201-0700 [TRACE] plugin.stdio: waiting for stdio data
```
While doing some integration testing I was getting errors saying the user data was invalid bas64.

Docs say...

```
// . If you are using a command line tool, base64-encoding is performed for you,
	// and you can load the text from a file. Otherwise, you must provide
	// base64-encoded text. User data is limited to 16 KB.
	UserData *string
```
@whywaita
Copy link
Owner

whywaita commented May 1, 2024

@alfred-stokespace Please rebase this Pull Request #3.

alfred-stokespace and others added 4 commits May 1, 2024 07:20
Wasn't sure if you wanted the baggage of an addtl test, so I'll put in the commit notes

```
func Test_EnvAmiOverride(t *testing.T) {
	ctx := context.Background()

	const alternateAmiLabel = "alternateAmiLabelPrefix:"
	envErr := os.Setenv(EnvShoesLabelAmiPrefix, alternateAmiLabel)
	if envErr != nil {
		t.Fatalf("faled to set env var")
	}
	defer func() {
		err := os.Unsetenv(EnvShoesLabelAmiPrefix)
		if err != nil {
			t.Fatalf("faled to un-set env var")
		}
	}()

	testEndpoint := testutils.GetTestEndpoint()

	a, err := newServer(ctx, testEndpoint)
	if err != nil {
		t.Fatalf("failed to newServer: %+v", err)
	}
	_, _, err = a.createRunnerInstance(ctx, "test-runner", "echo 0", pb.ResourceType_Nano, []string{alternateAmiLabel + "ami-0000"})
	if err != nil {
		t.Fatalf("failed to createRunnerInstance: %+v", err)
	}
}

```
that passed ...
```
=== RUN   Test_EnvAmiOverride
AMI used: ami-0000
--- PASS: Test_EnvAmiOverride (0.03s)
PASS
```
Test outputs
```
=== RUN   Test_createRunnerInstance
AMI used: ami-test
--- PASS: Test_createRunnerInstance (1.06s)
=== RUN   Test_deleteRunnerInstance
AMI used: ami-02868af3c3df4b3aa
--- PASS: Test_deleteRunnerInstance (0.64s)
PASS
```
also cleanup of grammar
@alfred-stokespace
Copy link
Author

@alfred-stokespace Please rebase this Pull Request #3.

tried to do this purely with git commands and ended up having git troubles...

git push -u origin patch-1
Enumerating objects: 9, done.
Counting objects: 100% (9/9), done.
Delta compression using up to 6 threads
Compressing objects: 100% (4/4), done.
Writing objects: 100% (5/5), 475 bytes | 475.00 KiB/s, done.
Total 5 (delta 2), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (2/2), completed with 2 local objects.
To https://github.com/alfred-stokespace/shoes-aws.git
 ! [remote rejected] patch-1 -> patch-1 (refusing to allow a Personal Access Token to create or update workflow `.github/workflows/build.yaml` without `workflow` scope)
error: failed to push some refs to 'https://github.com/alfred-stokespace/shoes-aws.git'

Force push to my repo branch failed as well, odd. So I did fork sync on the website, which I think ended up performing a merge commit, I don't think that's what you wanted. Sorry.

@alfred-stokespace
Copy link
Author

@whywaita I'm now far enough along in test that I see a problem back in the myshoes project that is blocking this from completely working...

https://github.com/whywaita/myshoes/blob/603c51b07c194f6d0e98218b65a5568ce8e3fc2c/pkg/starter/scripts.go#L79

Since that labels array is empty, it gets built into the compressed script with only ever two entries, myshoes and dependabot.

That has the downside of preventing the GH scheduler from scheduling the job to the runner.

I'm working on changes to the myshoes project so that labels from the job's json get pulled out and passed through a few functions to end up populating that labels array.

I see that in the script template you assume myshoes I'll make sure to filter out that label so it's not doubled up on.

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.

2 participants