Skip to content
This repository was archived by the owner on Aug 3, 2026. It is now read-only.

FAL-2260 Prepare opencraft release branch for lilac.2 - #175

Merged
samuelallan72 merged 8 commits into
opencraft-release/lilac.2from
samuel/prepare-lilac.2-release
Oct 25, 2021
Merged

FAL-2260 Prepare opencraft release branch for lilac.2#175
samuelallan72 merged 8 commits into
opencraft-release/lilac.2from
samuel/prepare-lilac.2-release

Conversation

@samuelallan72

@samuelallan72 samuelallan72 commented Oct 5, 2021

Copy link
Copy Markdown
Member

FAL-2260

Port the following PRs / commits from koa to lilac:

These are skipped because they have already been upstreamed in lilac 🎉 :

And this isn't required any more (see comments below):

Test instructions:

For each patch from edx/configuration@open-release/koa.master...open-craft:opencraft-release/koa.3 , verify that it has either been upstreamed or ported cleanly.

Launch a lilac instance with ocim, using this configuration branch, and verify everything is deployed successfully. (wip - deploying on https://manage.opencraft.com/instance/29580/ )

Notes / Concerns:

@samuelallan72 samuelallan72 self-assigned this Oct 5, 2021
@samuelallan72
samuelallan72 force-pushed the samuel/prepare-lilac.2-release branch from 8e54316 to 082fa19 Compare October 5, 2021 05:58
@samuelallan72 samuelallan72 changed the title WIP: FAL-2260 Prepare opencraft release branch for lilac.2 FAL-2260 Prepare opencraft release branch for lilac.2 Oct 11, 2021
Comment thread playbooks/retire_host.yml
@michaelbukachi

Copy link
Copy Markdown

I don't see the changes from [SE-4304] [SE-4495] feat: add celerybeat configuration.

@samuelallan72

Copy link
Copy Markdown
Member Author

@michaelbukachi

I don't see the changes from [SE-4304] [SE-4495] feat: add celerybeat configuration.

This is this commit: dd2f2ac The diff shows less changes than the original commit, because most of it has been upstreamed.

CC @gabor-boros do we need the 'install simple-beat to control celerybeat processes' task? Will it be upstreamed?

@michaelbukachi

Copy link
Copy Markdown

@swalladge looks good.

@gabor-boros

Copy link
Copy Markdown

@swalladge I'm a bit confused by your question. The changes are upstreamed as part of

Am I missing something here?

@samuelallan72

Copy link
Copy Markdown
Member Author

@gabor-boros

I'm a bit confused by your question. The changes are upstreamed as part of ...

Am I missing something here?

aaah sorry, I was the one missing something. :P Yep it's all been upstreamed; git was playing tricks on me when I checked the diff and applied the commit though. 🤦‍♂️

@samuelallan72
samuelallan72 force-pushed the samuel/prepare-lilac.2-release branch from 93ba25e to 188133c Compare October 13, 2021 22:18
@gabor-boros

Copy link
Copy Markdown

@swalladge I believe some PRs/commits are missing. Correct me if I overlooked it, but OCIM now can provision instances that are using multi tenant redis. Now, the celery upgrade is not upstreaming yet as I'm still fighting with the ticket in the current sprint.

Taking that into account, shouldn't we port that to opencraft lilac as well? Since it is something that will never end up on edx/lilac but we want to use it, I guess we should cherry-pick that too.

Cc: @pomegranited

@samuelallan72

Copy link
Copy Markdown
Member Author

@gabor-boros

I believe some PRs/commits are missing. Correct me if I overlooked it, but OCIM now can provision instances that are using multi tenant redis. Now, the celery upgrade is not upstreaming yet as I'm still fighting with the ticket in the current sprint.

Taking that into account, shouldn't we port that to opencraft lilac as well?

Sure yes definitely. Are these commits/PRs you're talking about on edx/configuration and in koa? I didn't see any other commits related to redis/celery. If they're in edx/configuration master, then we can backport to our lilac branch after this commit is merged (ie. after we actually cut our lilac release).

@gabor-boros

Copy link
Copy Markdown

@gabor-boros

I believe some PRs/commits are missing. Correct me if I overlooked it, but OCIM now can provision instances that are using multi tenant redis. Now, the celery upgrade is not upstreaming yet as I'm still fighting with the ticket in the current sprint.

Taking that into account, shouldn't we port that to opencraft lilac as well?

Sure yes definitely. Are these commits/PRs you're talking about on edx/configuration and in koa? I didn't see any other commits related to redis/celery. If they're in edx/configuration master, then we can backport to our lilac branch after this commit is merged (ie. after we actually cut our lilac release).

Well, it seems I remembered wrongly. Sorry about the confusion!

@pkulkark

Copy link
Copy Markdown
Member

@swalladge

I couldn't find an associated PR for edx@4df1323 CC @pkulkark could you check this?

I believe that was a backport of this upstream PR: https://github.com/edx/configuration/pull/6465.

@samuelallan72

Copy link
Copy Markdown
Member Author

@pkulkark

I believe that was a backport of this upstream PR: edx#6465.

Ok. Was there no corresponding PR for the backport?

@kaizoku

kaizoku commented Oct 18, 2021

Copy link
Copy Markdown

@swalladge, for #140 , the AppleIdAuth backend is in lilac now, so that particular PR shouldn't be necessary.
I do see they noted that the path changed, which is reflected in lms/envs/production.py, so we'll need to updated those paths for EDXAPP_THIRD_PARTY_AUTH_BACKENDS in playbooks/roles/edxapp/defaults/main.yml too.

0x29a and others added 7 commits October 19, 2021 10:50
This PR add the flags to sync the edx services logs to s3 bucket.

(cherry picked from commit eb402aa)
…ls_extra_requirements_koa

feature: Allow installing extra requirements when using the `credentials` role
(cherry picked from commit 7ce36c7)
…ygithub-fix

[FAL-2409] Use the tubular pygithub fix in our common branch

(cherry picked from commit a957ce3)
@samuelallan72
samuelallan72 force-pushed the samuel/prepare-lilac.2-release branch from 188133c to b5eeb9b Compare October 19, 2021 00:20
@samuelallan72

Copy link
Copy Markdown
Member Author

@kaizoku

the AppleIdAuth backend is in lilac now, so that particular PR shouldn't be necessary.

Awesome! :)

I do see they noted that the path changed, which is reflected in lms/envs/production.py, so we'll need to updated those paths for EDXAPP_THIRD_PARTY_AUTH_BACKENDS in playbooks/roles/edxapp/defaults/main.yml too.

Looks like that's already been done, so that's all good now.

Comment thread playbooks/openedx_native.yml Outdated
DOMAIN: "{{ COMMON_GIT_MIRROR }}"
PATH: "open-craft"
REPO: "tubular.git"
VERSION: "opencraft-release/koa.3"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@pomegranited what are your thoughts here? We can't use open-release/lilac.2 for tubular because https://github.com/edx/tubular/pull/531 didn't make it to lilac.2, but we should default to a lilac release here to avoid issues. Maybe we can default to open-release/lilac.master here?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@swalladge That sounds safe -- tubular isn't changed very often, so we're not risking much by using a potentially-shifting lilac.master target.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍 I updated in 06fb8f4 - would you mind sanity checking this?

I'm also deploying a new sandbox instance for testing. CC @michaelbukachi if you'd like to make another review pass now that there has been some changes.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@swalladge 06fb8f4 looks good 👍 Thanks for simplifying that drift too.

@michaelbukachi To test this, you can check the sandbox server for retirement-related errors in /edx/var/log/**/*.log, and verify that you're able to create and delete a user successfully.

Previously, we pinned to an opencraft release as a quick fix,
but that quick fix is now merged to lilac upstream.
(The fix being https://github.com/edx/tubular/pull/531 )
It didn't make it to lilac.2, so we need to pin to lilac.master.

@michaelbukachi michaelbukachi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LG

@samuelallan72
samuelallan72 merged commit 0252892 into opencraft-release/lilac.2 Oct 25, 2021
@samuelallan72
samuelallan72 deleted the samuel/prepare-lilac.2-release branch October 25, 2021 00:56
@pomegranited

pomegranited commented Oct 29, 2021

Copy link
Copy Markdown

@swalladge I don't see your change from https://github.com/edx/configuration/pull/6462 in this branch? It was partially backported to our koa.3 release branch by #176 but that port unfortunately doesn't include the EDXAPP_ENABLE_MONGODB_INDEXES and FORUM_ENABLE_MONGODB_INDEXES gates that your upstream PR included, creating some code drift.

I think https://github.com/edx/configuration/pull/6462 should be ported to this branch, and the *_ENABLE_MONGODB_INDEXES flags enabled in Ocim. Any client sites that want to use them will have to enable them too, CC @Agrendalath

I will include this in the scope of FAL-2543, which is semi-related.

@samuelallan72

Copy link
Copy Markdown
Member Author

@pomegranited

I don't see your change from edx#6462 in this branch?

Yeah they wouldn't be unless we already had backported it to our koa branch, or upstream had included it in lilac.2...

I think edx#6462 should be ported to this branch, and the *_ENABLE_MONGODB_INDEXES flags enabled in Ocim

Sounds like a good plan 👍

I will include this in the scope of FAL-2543, which is semi-related.

Thanks! :D

@Agrendalath

Copy link
Copy Markdown
Member

@pomegranited, @swalladge, huh, I had no idea we've created two tickets for this 🤦

@samuelallan72

Copy link
Copy Markdown
Member Author

@Agrendalath oh haha I didn't even notice they were different tickets. Looks like we forgot to backport to the client branch in the first ticket, and the second ticket didn't notice that we'd already implemented it upstream.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants