-
Notifications
You must be signed in to change notification settings - Fork 168
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
clarify deploy-full-rp-service-in-dev.md guide #2964
Conversation
Publishing as a draft for visibility, I still need to polish this a bit. Feedback welcome. |
2. During the deployment, don't edit files in your ARO-RP repository so that | ||
`git status` reports a clean working tree. Otherwise aro container image | ||
will have `-dirty` suffix, which can cause problems esp. when the repository | ||
becomes dirty during the procedure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we need to prevent this programmatically in this scenario because it can't be wanted. Unless somebody wanna deploy an RP using some not committed changes.
I would add some checks or warning messages to the scripts we use or to make's targets.
But we can do it in another PR. Does it make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would make sense. Actually I don't know about a way how to get an image with *-dirty
tag working, it's just not very obvious what the problem is when you run into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What sort of problems does the -dirty suffix cause? Is it only an issue when deploying a shared rp or is it an issue for local dev as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are few problems which one could run into:
- if the working tree becomes dirty during the process (eg. because you create a temporary helper script to run some of the setup), you could end up with different image tag pushed in the azure image registry compared to the tag expected by aro deployer
- with a dirty tag, it's not clear what's actually in the image
- I failed to configure the deployment process to work with
*-dirty
images when I run into this, there likely is a way to handle this, but it's not documented and it was not worth for me to figure this out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One solution would be to set COMMIT/TAG = latest if RP_MODE == development, this will make reruns or testing a lot easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it's more of a recommendation. You can definitely deploy with the dirty tag if you want (I do it and sometimes it's an accident but it still works), but you will have the potential issues mentioned above.
Should we just change the wording to "it's recommended to..." and add the content here to the document? https://github.com/Azure/ARO-RP/pull/2964/files#r1238448125
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the text as suggested by @cadenmarchese .
010eaeb
to
af0a886
Compare
The changes are ready for review. I addressed the feedback about the dirty image tag, and tweaked the text about container registry used for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/azp run ci |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run ci |
Azure Pipelines successfully started running 1 pipeline(s). |
Rebased on top of current master (14e3c5a) to unblock the github checks. |
/azp run ci |
Azure Pipelines successfully started running 1 pipeline(s). |
Which issue this PR addresses:
None.
What this PR does / why we need it:
Improves "Deploy an Entire RP Development Service" guide so that one can avoid problems caused by misunderstanding of the procedure.
Test plan for issue:
Changes are based on my experience with this procedure. No further testing expected beyond review.