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

Fix for Service and Source connection (both source repo and docker image) #21

Merged

Conversation

wertlex
Copy link
Contributor

@wertlex wertlex commented Feb 27, 2024

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 or master) when a branch was not specified. However, during this journey, I found that this was too impractical for a couple of reasons:

  1. it was making some assumptions regarding the branch naming ( master vs main ) problem
  2. even worse, it was doing that assumption rather silently using computed variables (yes, one has to keep an eye on generated plans, but ... you know)

So 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 when source_repo is specified. For existing TF configurations without a source_repo_branch it will lead to the error being rendered in the planning phase, so there will be no room for unintentional changes

Service 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 and source_repo_branch are also demanding to be specified together if at least one of them is specified

Testing

Unfortunately, I had not enough time to build really good automated tests, so I performed some intensive manual testing. My go-to config:

terraform {
  required_providers {
    railway = {
       # source  = "terraform-community-providers/railway"
       # requires `provider_installation.dev_overrides` in `~/.terraformrc` defined properly
       source  = "terraform-community-providers/railway-dev"
       version = "0.3.1"
    }
  }
}

provider "railway" {
  token = "<your token goes here>"
}

resource "railway_project" "tf-example" {
  name = "my-project-1"
}

resource "railway_service" "service-1" {
  name       = "service-1"
  project_id = railway_project.tf-example.id
# uncomment to use repo or image
#  source_repo = "<your repo goes here>"
#  source_repo_branch = "railway"
#  root_directory = "/home/user"
#  source_image = "hello-world"
}

Fixes #16

@pksunkara
Copy link
Contributor

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.

@wertlex
Copy link
Contributor Author

wertlex commented Feb 28, 2024

Thank you @pksunkara !
Let me know if there are some things I could help you with here

@pksunkara pksunkara self-assigned this Mar 2, 2024
@pksunkara
Copy link
Contributor

@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?

@wertlex
Copy link
Contributor Author

wertlex commented Mar 4, 2024

@pksunkara is there any branch where I could take a look on the resource_service_test.go ?

@pksunkara
Copy link
Contributor

I pushed to this branch.

Command for running the test:

env TF_ACC=1 go test -run '^TestAccServiceResourceNonDefaultImage$' -v ./...

@wertlex
Copy link
Contributor Author

wertlex commented Mar 4, 2024

yep, will take a look

@wertlex
Copy link
Contributor Author

wertlex commented Mar 4, 2024

So, it seems like there are couple things to deal with:

  1. It turns out that the root cause was here

  2. 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

@wertlex
Copy link
Contributor Author

wertlex commented Mar 4, 2024

@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

@pksunkara
Copy link
Contributor

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.

@pksunkara
Copy link
Contributor

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?

@pksunkara pksunkara merged commit 3bd4679 into terraform-community-providers:master Mar 4, 2024
2 of 3 checks passed
@wertlex
Copy link
Contributor Author

wertlex commented Mar 5, 2024

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 now
  • when branch is not specified it silently fails, since there is no default fallback value (main, master or whatever)
  • there are still GraphQL mutations which are not allowing to specify branch at all

But these are just my guesses. I was trying to get some information about this in Railway Discord, but didn't had any luck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Source Repo doesn't connect
2 participants