Closed wertlex closed 6 months ago
Looks like there might be a few potential issues with the implementation and the tests are missing too. I can take over, finish, and merge this on Sunday.
Thank you @pksunkara ! Let me know if there are some things I could help you with here
@wertlex I took a look over the weekend. I seem to getting something weird:
? github.com/terraform-community-providers/terraform-provider-railway [no test files]
=== RUN TestAccServiceResourceNonDefaultImage
resource_service_test.go:122: Step 1/5 error: Error running apply: exit status 1
Error: Client Error
with railway_service.test,
on terraform_plugin_test.tf line 2, in resource "railway_service" "test":
2: resource "railway_service" "test" {
Unable to read service settings, got error: input:3: serviceInstance
ServiceInstance not found
--- FAIL: TestAccServiceResourceNonDefaultImage (1.83s)
FAIL
FAIL github.com/terraform-community-providers/terraform-provider-railway/internal/provider 1.832s
FAIL
Looks like railway is not creating the service instances for the environments after we create the service. Can you look into this and see if you are getting the same?
@pksunkara is there any branch where I could take a look on the resource_service_test.go
?
I pushed to this branch.
Command for running the test:
env TF_ACC=1 go test -run '^TestAccServiceResourceNonDefaultImage$' -v ./...
yep, will take a look
So, it seems like there are couple things to deal with:
It turns out that the root cause was here
There is also something happening with cron_schedule
. I'll take a look there as well.
=== RUN TestAccServiceResourceNonDefaultImage
resource_service_test.go:122: Step 4/5 error: Error running apply: exit status 1
Error: Provider produced inconsistent result after apply
When applying changes to railway_service.test, provider
"provider[\"registry.terraform.io/hashicorp/railway\"]" produced an
unexpected new value: .cron_schedule: was null, but now cty.StringVal("0 0 *
* *").
This is a bug in the provider, which should be reported in the provider's own
issue tracker.
--- FAIL: TestAccServiceResourceNonDefaultImage (29.47s)
FAIL
@pksunkara performed some updates for the parameters (an idea of making them a pointers was looking good at glance 😅 ). Some basic test are passing, though I didn't ran whole test suite, since some ids are hardcoded. I'll be able to do that tomorrow though
P.S. probably you'll be able to run CI/CD pipeline till that time
Everything works. I wasn't able to properly make sure that the source_repo
works though because of the following error:
=== RUN TestAccServiceResourceNonDefaultRepo
resource_service_test.go:192: Step 1/5 error: Error running apply: exit status 1
Error: Client Error
with railway_service.test,
on terraform_plugin_test.tf line 2, in resource "railway_service" "test":
2: resource "railway_service" "test" {
Unable to connect repo or image to service, got error: input:3:
serviceConnect User does not have access to the repo
but that's probably because I don't have github connected to the test account.
But I am quite confused about why they added branch
though. Wouldn't people want to link different branches to different environments? So, why did they ask for default branch?
but that's probably because I don't have github connected to the test account. yes, that's true. One have to link github account to the profile in order to make it work
But I am quite confused about why they added branch though. Wouldn't people want to link different branches to different environments? So, why did they ask for default branch?
Well, in my eyes it is more like they made a decision to require branch
to be explicitly specified (which is understandable), but not propagated these changes through out the whole API. So, it ended up with the following state:
branch
must be specified nowbranch
is not specified it silently fails, since there is no default fallback value (main
, master
or whatever)branch
at allBut these are just my guesses. I was trying to get some information about this in Railway Discord, but didn't had any luck.
Hey guys!
Motivation
It seems like the only valid way to attach the source (both repo and image) to the Railway Service is by using the
serviceConnect
method. While other methods still seem to work, in fact, they are doing only part of the job, for instance by assigning a docker image, but not triggering a deployment.I performed a series of experiments (I wish Railway docs would be better, but..) and moved both repo and service connectivity to that new method. The same thing is relevant for disconnecting service from a repo or image:
serviceDisconnect
seems to be the only vital option right away.Branch name is a required property when using the git repo
An additional thing, that initially triggered me to jump into these deep waters is actually the fact that starting from the beginning of the year 2024, the git repository branch became a mandatory parameter. While omitting the branch was not leading to explicit error returned by API, all the calls with git repo set, but no branch specified were leading to silent failure.
I tried different approaches to deal with it, starting from the most obvious one: falling back to the default branch name (
main
ormaster
) when a branch was not specified. However, during this journey, I found that this was too impractical for a couple of reasons:master
vsmain
) problemSo in order not to introduce any real troubles I decided that the most straight option would be to demand
source_repo_branch
to be explicitly specified whensource_repo
is specified. For existing TF configurations without asource_repo_branch
it will lead to the error being rendered in the planning phase, so there will be no room for unintentional changesService schema refined
To cover all these things I decided to refine the Schema for Service just a little bit:
source_image
is checked to be mutually exclusive with all source repo parameters (they are not working anyway when both are provided)source_repo
andsource_repo_branch
are also demanding to be specified together if at least one of them is specifiedTesting
Unfortunately, I had not enough time to build really good automated tests, so I performed some intensive manual testing. My go-to config:
Fixes #16