Rename Discussion to Welcome Session, remove deprecated TrainingRequirements #2420

elichad · 2023-05-16T13:29:17Z

Noticed that these PRs aren't checked by CI as they're not against develop - will think about the best way to approach this

Fixes #2418
Fixes #2410

elichad · 2023-05-17T12:30:02Z

I added some migration tests to this PR using django-test-migrations - I think this is worth doing in cases where we're modifying database records during migration (i.e. not just changing fields).

Migration tests do run pretty slowly, so I modified our existing test setup to ignore them by default, but have them callable when needed.

pbanaszkiewicz

I added some comments and I think we should discuss the approach regarding CI/CD pipeline and data migration vs seeding.

pbanaszkiewicz · 2023-05-24T20:09:15Z

.github/workflows/cicd_main.yml

@@ -6,7 +6,7 @@ on:
  push:
    branches: [ main ]
  pull_request:
-    branches: [ main ]


File cicd_develop.yml should be invoked for all PRs, I think. I'm not sure what this change does compared to other pipelines.

This PR is against feature/instructor-checkout-changes rather than develop, so needs explicit inclusion in a pipeline somewhere. I'm not sure if cicd_main.yml is the best place to do this from a semantic perspective, it was just the most convenient

@elichad What is your purpose here? Do you want to be able to run complete CI/CD against a feature branch, or do you just need a CI against a feature branch?

In any case, I'd create a new file .github/workflows/cicd_feature_branch.yml and make sure it is set against particular branch (ie. with no wildcard). We could control this pipeline from individual feature branches, for example we could easily switch off whole pipeline by changing the branches parameter, or run only tests by disabling "CD" part in that individual file.

I would keep cicd_main.yml untouched for now, as we want to eventually add proper CI/CD and infrastructure to the main branch, too.

.github/workflows/test.yml

pbanaszkiewicz · 2023-05-24T20:14:00Z

amy/scripts/seed_training_requirements.py

+    "DC Demo",
+    "SWC Demo",
+    "LC Demo",
+]


Just want to make sure: were these migrated in production? I'm not sure what would happen if these were removed for existing users.

Okay, I saw a migration removing these entries. We should not keep 2 systems for managing "static" (seeded) objects. I would suggest sticking to the seeding script, unless a data migration is actually required.

Okay, I can remove that step from the migration. This is one thing I am uncertain of - are the seeding scripts run when deploying new versions to production?

Good question. Let me see... I think they are not run during production deployment (but they definitely are run when deploying to test stage).

We can run them manually after deploying to production, or make sure there's a step that runs them in Ansible.

Once we switch to Docker-based production, we will run the seeds just like we do in test-amy.

pbanaszkiewicz

In general looks good, but I would like to:

see a new pipeline for feature branches
decide how we are going to approach running seeding in production
discuss the migration 0259 (what is required and what is not).

pbanaszkiewicz · 2023-06-08T15:37:29Z

.github/workflows/cicd_main.yml

@@ -6,7 +6,7 @@ on:
  push:
    branches: [ main ]
  pull_request:
-    branches: [ main ]


@elichad What is your purpose here? Do you want to be able to run complete CI/CD against a feature branch, or do you just need a CI against a feature branch?

In any case, I'd create a new file .github/workflows/cicd_feature_branch.yml and make sure it is set against particular branch (ie. with no wildcard). We could control this pipeline from individual feature branches, for example we could easily switch off whole pipeline by changing the branches parameter, or run only tests by disabling "CD" part in that individual file.

I would keep cicd_main.yml untouched for now, as we want to eventually add proper CI/CD and infrastructure to the main branch, too.

pbanaszkiewicz · 2023-06-08T15:54:26Z

amy/scripts/seed_training_requirements.py

+    "DC Demo",
+    "SWC Demo",
+    "LC Demo",
+]


Good question. Let me see... I think they are not run during production deployment (but they definitely are run when deploying to test stage).

We can run them manually after deploying to production, or make sure there's a step that runs them in Ansible.

Once we switch to Docker-based production, we will run the seeds just like we do in test-amy.

pbanaszkiewicz · 2023-06-08T16:03:43Z

amy/scripts/seed_training_requirements.py

@@ -22,13 +29,7 @@

 TRAINING_REQUIREMENTS: list[TrainingRequirementDef] = [
    {"name": "Training", "url_required": False, "event_required": True},
-    {"name": "DC Homework", "url_required": True, "event_required": False},
-    {"name": "SWC Homework", "url_required": True, "event_required": False},
-    {"name": "Discussion", "url_required": False, "event_required": False},


"Discussion" is not being deprecated as it's being switched to "Welcome Session". The migration 0259_... should probably only migrate the name "Discussion" <-> "Welcome Session", without changes the seeding manages.

pbanaszkiewicz · 2023-06-08T16:12:34Z

amy/workshops/migrations/0259_remove_deprecated_training_requirements.py

+    TrainingProgress = apps.get_model("workshops", "TrainingProgress")
+
+    # migrate SWC/DC/LC specific progress to generic demo/lesson contribution
+    # this was already done in production AMY but not in local databases


What do you mean by "local databases"? Do you mean development databases, or test-amy2 database?

This migration looks reasonable (except for lines 52-54 where Training Requirements are removed), but I'm trying to understand if it's truly needed.

Development databases. When fake training progresses are created, they choose from all available TrainingRequirements, so my local dev database was populated with lots of SWC Demos, LC Homeworks, etc.
We don't have auto-generated fake data on test-amy2 so this migration isn't required there (though it would have been required if we had generated some).

I've removed the creation/deletion of TrainingRequirements from this step of the migration since that's covered by the seeding, but this section still remains for discussion.

pbanaszkiewicz · 2023-06-20T19:55:54Z

.github/workflows/cicd_feature_branch.yml

@@ -0,0 +1,15 @@
+# CI for feature branches - contains only test runs
+
+name: CI (feature)


pbanaszkiewicz

Looks good. I had only 1 comment. But we have to remember about seeding in production before deploying v4.2.

pbanaszkiewicz · 2023-06-20T20:04:10Z

amy/workshops/models.py

-            "LC Homework",
-        ]
-        DEMO_TRAININGPROGRESS_NAMES = ["Demo", "SWC Demo", "DC Demo", "LC Demo"]
+        LESSON_CONTRIBUTION_NAMES = ["Lesson Contribution"]

        return self.annotate(
            passed_training=passed("Training"),
            passed_lesson_contribution=passed_either(*LESSON_CONTRIBUTION_NAMES),


Can we use passed instead of passed_either with literal string "Lesson Contribution" as an argument?

Yes - this is coming in a later PR (not yet open) which updates the checkout requirements after the change from "Lesson Contribution" -> "Get Involved"

elichad added 7 commits May 16, 2023 12:50

rename discussion to welcome session

f02c991

better naming for reverse migration function

cfe102d

deprecate DC/SWC/LC specific homework and demo

01b5a5b

migrate LP-specific progress to generic

4bc5997

remove references to LP-specific checkout

0fd20b1

rebase updates, don't re-create requirements

037b8b9

add migration test

22018e3

elichad changed the title ~~Rename Discussion to Welcome Session, remove deprecated~~ Rename Discussion to Welcome Session, remove deprecated TrainingRequirements May 16, 2023

elichad added 6 commits May 17, 2023 11:20

combine tests to reduce runtime

c19e99c

use assertEqual

aa01217

handle migration tests during testing

105e577

add CI checks to PRs against feature branches

bfc0e49

test renaming of discussion to welcome session

8b7f643

remove rogue s

f1c784a

elichad requested a review from pbanaszkiewicz May 17, 2023 12:31

elichad self-assigned this May 17, 2023

elichad added this to the v4.2 milestone May 17, 2023

This was linked to issues May 17, 2023

Rename Community Discussions -> Welcome Sessions #2410

Closed

Clean up unused options/fields in training requirements #2418

Closed

pbanaszkiewicz reviewed May 24, 2023

View reviewed changes

elichad mentioned this pull request Jun 1, 2023

Create "Get Involved" requirement and associated Involvement model #2431

Merged

pbanaszkiewicz reviewed Jun 8, 2023

View reviewed changes

elichad added 3 commits June 19, 2023 12:38

make a specific workflow for feature branch CI/CD

04196fb

remove creation/deletion of TrainingRequirements from migration

a5708da

update migration tests

a6288ed

pbanaszkiewicz self-requested a review June 19, 2023 16:24

pbanaszkiewicz reviewed Jun 20, 2023

View reviewed changes

pbanaszkiewicz approved these changes Jun 20, 2023

View reviewed changes

elichad merged commit 027dc20 into feature/instructor-checkout-changes Jun 21, 2023

elichad deleted the update-training-requirements branch June 21, 2023 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename Discussion to Welcome Session, remove deprecated TrainingRequirements #2420

Rename Discussion to Welcome Session, remove deprecated TrainingRequirements #2420

elichad commented May 16, 2023 •

edited

Loading

elichad commented May 17, 2023

pbanaszkiewicz left a comment

pbanaszkiewicz May 24, 2023

elichad May 30, 2023 •

edited

Loading

pbanaszkiewicz Jun 8, 2023

pbanaszkiewicz May 24, 2023

pbanaszkiewicz May 24, 2023

elichad May 30, 2023

pbanaszkiewicz Jun 8, 2023

pbanaszkiewicz left a comment

pbanaszkiewicz Jun 8, 2023

pbanaszkiewicz Jun 8, 2023

pbanaszkiewicz Jun 8, 2023

pbanaszkiewicz Jun 8, 2023

elichad Jun 19, 2023

elichad Jun 19, 2023

pbanaszkiewicz Jun 20, 2023

pbanaszkiewicz left a comment

pbanaszkiewicz Jun 20, 2023

elichad Jun 21, 2023 •

edited

Loading

		@@ -0,0 +1,15 @@
		# CI for feature branches - contains only test runs

		name: CI (feature)

Rename Discussion to Welcome Session, remove deprecated TrainingRequirements #2420

Rename Discussion to Welcome Session, remove deprecated TrainingRequirements #2420

Conversation

elichad commented May 16, 2023 • edited Loading

elichad commented May 17, 2023

pbanaszkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichad May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbanaszkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbanaszkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichad Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

elichad commented May 16, 2023 •

edited

Loading

elichad May 30, 2023 •

edited

Loading

elichad Jun 21, 2023 •

edited

Loading