Skip to content

Fix the default executor host since it's changed in 1.4 - #6098

Closed
stnguyen90 wants to merge 2 commits into
1.4.xfrom
fix-default-executor-host
Closed

stnguyen90 wants to merge 2 commits into
1.4.xfrom
fix-default-executor-host

Conversation

@stnguyen90

Copy link
Copy Markdown
Contributor

What does this PR do?

Since the executor's hostname is now executor

openruntimes-executor:
container_name: openruntimes-executor
hostname: executor
<<: *x-logging

the default variable value should be updated.

Test Plan

Manual

Related PRs and Issues

None

Checklist

  • Have you read the Contributing Guidelines on issues?
  • If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

@stnguyen90 stnguyen90 added this to the 1.4.2 milestone Aug 31, 2023
@stnguyen90
stnguyen90 marked this pull request as ready for review August 31, 2023 20:41
@abnegate

abnegate commented Sep 1, 2023

Copy link
Copy Markdown
Member

I think we'll need to change the Dockerfile default as well

@stnguyen90

Copy link
Copy Markdown
Contributor Author

@abnegate, the env var in the Dockerfile shouldn't matter after #5971

@stnguyen90 stnguyen90 modified the milestones: 1.4.2, 1.4.3 Sep 2, 2023
Comment thread app/config/variables.php
'description' => 'The host used by Appwrite to communicate with the function executor!',
'introduction' => '0.13.0',
'default' => 'http://appwrite-executor/v1',
'default' => 'http://executor/v1',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, but maybe we should go back to using appwrite-executor, or have smart migration script (if value is exactly http://appwrite-executor/v1, then replace. If not, keep using their modified value)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

let's leave it as executor. smarter installation command coming in: #6137

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@stnguyen90 the override PR has been merged. I think now we should change it back to appwrite-executor

@stnguyen90 stnguyen90 modified the milestones: 1.4.3, 1.4.2 Sep 4, 2023
@stnguyen90
stnguyen90 requested a review from Meldiron September 4, 2023 16:43
@stnguyen90 stnguyen90 closed this Sep 5, 2023
@stnguyen90
stnguyen90 deleted the fix-default-executor-host branch September 5, 2023 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants