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: migrate job #154

Merged
merged 2 commits into from
Jan 19, 2023
Merged

fix: migrate job #154

merged 2 commits into from
Jan 19, 2023

Conversation

revant
Copy link
Collaborator

@revant revant commented Jan 19, 2023

fixes #153

@revant
Copy link
Collaborator Author

revant commented Jan 19, 2023

@tarioch this seems to have fixed the problem. I don't know why = caused a problem.

can you try it in your job yaml, If it works I'll merge this.

@revant
Copy link
Collaborator Author

revant commented Jan 19, 2023

Running bench migrate without --site option works fine, so I guess that argument needs to be dropped

AFAIK It should work only if currentsite.txt has a site set. It is added by bench use {site-name} or using bench new-site --set-default ...

In case of multiple sites under a bench, the option is required to specify which site to run the bench command for.

There is also a magic keyword all which can be used to run command for all sites bench --site all ...

@tarioch
Copy link

tarioch commented Jan 19, 2023

Doesn't look like it works. Still get no such option error.

@revant
Copy link
Collaborator Author

revant commented Jan 19, 2023

make sure you are using the template from PR.
or just make the changes to the generated yaml and apply.

I've added test to confirm the migration job from PR works.

migration output starts here: https://github.com/frappe/helm/actions/runs/3958254435/jobs/6779598616#step:3:302

@tarioch
Copy link

tarioch commented Jan 19, 2023

I'm using the option from the values

jobs:
  migrate:
    enabled: true
    siteName: "mysite.domain.tld"

@tarioch
Copy link

tarioch commented Jan 19, 2023

Maybe it only works if using the erpnext image and not with the custom ones built according to https://github.com/frappe/frappe_docker/blob/main/docs/custom-apps.md

@revant
Copy link
Collaborator Author

revant commented Jan 19, 2023

You generate file?

the pipe command which does the | kubectl apply?

that is generating the file.

don't pipe it to kubectl apply.

pipe it to a file and edit it and replace = with space.

Then kubectl -n erpnext apply -f /path/to/file.yaml

@tarioch
Copy link

tarioch commented Jan 19, 2023

no, I'm just doing a helm install/upgrade, not using kubectl manually

@revant
Copy link
Collaborator Author

revant commented Jan 19, 2023

Maybe it only works if using the erpnext image and not with the custom ones built according to https://github.com/frappe/frappe_docker/blob/main/docs/custom-apps.md

Works with my custom app

https://gitlab.com/castlecraft/k8s_bench/-/jobs/3619771980

I used that app to confirm the fix.

Added the test because it will fail for erpnext in the current code.

@revant
Copy link
Collaborator Author

revant commented Jan 19, 2023

no, I'm just doing a helm install/upgrade, not using kubectl manually

for that you will need to wait for this PR to get merged and the next chart release get published.

The fix is on my fork! sent as a PR!

@revant revant merged commit 3f386ef into frappe:main Jan 19, 2023
@revant revant deleted the fix-153 branch January 19, 2023 12:28
@revant
Copy link
Collaborator Author

revant commented Jan 19, 2023

check the version 6.0.2 now.

you will need to helm repo update locally so it pulls the latest index.

@tarioch
Copy link

tarioch commented Jan 19, 2023

Can confirm that it works with 6.0.2, thank you very much

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.

Migrate job fails with new 6.0.1 custom app
2 participants