[Issue/3270] Make docker context path be the parent of the beamable config folder #3374
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gabrielbeamable
commented
May 9, 2024
cli/cli/Services/BeamoLocalSystem.cs
Outdated
| @@ -353,9 +359,14 @@ private async Promise UpdateDockerFile(string serviceName) | |||
|
|
|||
| foreach (var dependency in dependencies) | |||
| { | |||
| string depName = dependency.name; | |||
| newText = newText.Replace(endTag, replacement.Replace(serviceNameTag, depName)); | |||
| string path = _configService.GetRelativeToBeamableFolderPath(dependency.projPath); | |||
There was a problem hiding this comment.
this should be relative to the docker context path.
| @@ -309,11 +309,12 @@ public async Task AddProjectDependency(BeamoServiceDefinition project, BeamoServ | |||
| public static bool ValidateBeamoServiceId_DoesntExists(string beamoServiceId, List<BeamoServiceDefinition> serviceDefinitions) => | |||
| !serviceDefinitions.Contains(new BeamoServiceDefinition() { BeamoId = beamoServiceId }, new BeamoServiceDefinition.IdEquality()); | |||
|
|
|||
| private async Promise UpdateDockerFile(string serviceName) | |||
| public async Promise UpdateDockerFile(BeamoServiceDefinition serviceDefinition) | |||
There was a problem hiding this comment.
call this from services run and services deploy
| { | ||
| if (line.StartsWith("COPY")) | ||
| { | ||
| var parts = line.Split(" "); |
There was a problem hiding this comment.
use remove empty entries here, and check if this array has COPY SOURCE DESTINATION, 3 elements, then it's correct. And throw specific exceptions that we care about in the GetAttributes call
| @@ -34,6 +34,11 @@ public partial class BeamoLocalSystem | |||
| BeamoProtocolType.HttpMicroservice, | |||
| async (definition, protocol) => | |||
| { | |||
| if (string.IsNullOrEmpty(buildContexPath)) | |||
| { | |||
| buildContexPath = _configService.GetDockerBuildContextPath(); | |||
There was a problem hiding this comment.
remove this parameter and just get the value from this method from ConfigService
…/3270 # Conflicts: # cli/cli/Services/ProjectService.cs
|
Lightbeam link |
|
Lightbeam link |
|
Lightbeam link |
|
Lightbeam link |
github-actions bot
pushed a commit
that referenced
this pull request
May 14, 2024
…e beamable config folder (#3374)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
resolves #3270
Brief Description
While working on fixing the
beam services runcommand, I found that because we use standalone microservices in different ways, for example in the Unity SDK, then we were previously, it means that the way we were handling the docker context path was wrong and would only work for what is inside the microservice folder, and not work with dependencies for example. This basically fixes that, making the docker context path be the parent folder of the beamable config folder in the project.Notes
When you are merging a feature branch into
main, please squash merge and make sure the final commit contains any relevent JIRA ticket number. If you are merging frommaintostaging, orstagingtoproduction, please use a regular merge commit.Does this introduce tech-debt? If so, have you added an entry to the Tech-debt document?