Fix: xblock ci workflow #412

Talha-Rizwan · 2023-11-28T14:42:02Z

[ complete ] Changelog record added
[ not applicable ] Documentation updated (not only docstrings)
[ complete ] Fixup commits are squashed away
[ not applicable ] Unit tests added/updated
[ not applicable ] Manual testing instructions provided
[ not applicable ] Noted any: Concerns, dependencies, deadlines, tickets

The CI workflow was throughing errors on creating a new xblock. This commit eliminates and corrects the error generating steps.

openedx-webhooks · 2023-11-28T14:42:08Z

Thanks for the pull request, @Talha-Rizwan! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

⚠️ We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation. Please see the CONTRIBUTING file for more information. If you've signed an agreement in the past, you may need to re-sign. See The New Home of the Open edX Codebase for details.

Once you've signed the CLA, please allow 1 business day for it to be processed. After this time, you can re-run the CLA check by adding a comment here that you have signed it. If the problem persists, you can tag the @openedx/cla-problems team in a comment on your PR for further assistance.

Talha-Rizwan · 2023-11-28T14:58:49Z

This PR is for #412 issue and checkout this openedx discussion to understand the issue better.

itsjeyd · 2023-11-29T07:55:33Z

Hey @Talha-Rizwan, thank you for this contribution!

As the bot says, the next step will be for you to submit a signed CLA. Once that's done and the build is green, we can line this PR up for engineering review.

Talha-Rizwan · 2023-11-30T10:23:00Z

I've indicated my institutional affiliation.

e0d · 2023-11-30T16:09:26Z

@Talha-Rizwan should you be covered under the Arbisoft CLA? If so, can you ask the HR team to ask that you are added to the CRM?

Talha-Rizwan · 2023-11-30T16:49:03Z

Yes, I should be covered under the Arbisoft CLA. I've talked to HR team and they confirmed me that i've been added. Waiting for them to add me.

rgraber

A few questions

CHANGELOG.rst

rgraber · 2023-12-04T18:23:19Z

cookiecutter-xblock/{{cookiecutter.repo_name}}/tox.ini

-DJANGO_SETTINGS_MODULE = translation_settings
-addopts = --cov {{ cookiecutter.package_name }} --cov-report term-missing --cov-report xml
+DJANGO_SETTINGS_MODULE = myxblock.settings.test
+addopts = --cov myxblock --cov-report term-missing --cov-report xml


Why are we hardcoding this?

It automatically hardcodes the name of xblock and replaces {{ cookiecutter.package_name }} which in my case is myxblock , once you use this xblock template to create a new xblock. (This is one of the initial steps)

I understand why it does that when creating a new xblock, but doesn't this remove that from the template? won't this mean any new tox.ini files will be created with 'myxblock'?

Yeah you're right, i must have mistakenly changed it. replaced it back to original.

rgraber · 2023-12-04T18:26:02Z

cookiecutter-xblock/{{cookiecutter.repo_name}}/.github/workflows/ci.yml

    - name: Run Tests
      env:
        TOXENV: ${{ matrix.toxenv }}
      run: tox
-
-    - name: Run coverage


I understand taking out the build, but taking out codecov seems wrong

You're right about this but the workflow also needs to pass which in that case won't. So, we can add the codecov in such a way to make sure that the workflow still works.

Can you put this bit back? We've filed #417 as a separate issue to look at why it fails. We should still remove the other one you took out.

Also if you have any more details on how the codecov test fails, please add to the issue.

Sure, i'll do it.

e0d · 2023-12-05T12:19:26Z

cla check

Fix: replaced package name in tox.ini

nedbat · 2023-12-08T16:58:48Z

cookiecutter-xblock/{{cookiecutter.repo_name}}/.github/workflows/ci.yml

@@ -48,4 +41,4 @@ jobs:
      uses: codecov/codecov-action@v3
      with:
        flags: unittests
-        fail_ci_if_error: true
+        fail_ci_if_error: true


We use .editorconfig files to make editor settings the same among collaborators. If you enable editorconfig support in your editor or IDE, these kinds of accidental changes won't happen. https://editorconfig.org/

nedbat · 2023-12-08T16:59:39Z

CHANGELOG.rst

@@ -4,6 +4,13 @@ Change Log
 ..
   This file loosely adheres to the structure of https://keepachangelog.com/,
   but in reStructuredText instead of Markdown.
+2023-11-28


Suggested change

2023-11-28

2023-11-28

nedbat · 2023-12-08T17:06:15Z

cookiecutter-xblock/{{cookiecutter.repo_name}}/tox.ini

@@ -27,29 +27,21 @@ max-line-length = 120
 ; D412 = No blank lines allowed between a section header and its content (numpy style)
 ; D413 = Missing blank line after last section (numpy style)
 ; D414 = Section has no content (numpy style)
-ignore = D101,D200,D203,D212,D215,D404,D405,D406,D407,D408,D409,D410,D411,D412,D413,D414
+ignore = D101,D200,D203,D212,D215,D404,D405,D406,D407,D408,D409,D410,D411,D412,D413,D414,D401,D205 


If the pydocstyle checks are failing because of imperative mood and blank lines, we can fix that by correcting the docstrings in the generated repo, which I think would be a better fix.

BTW, when adding to exclusions like this, keep the numbers in sorted order, and include the comments above that explain the ignored rule.

rgraber

Sorry for the multiple rounds of reviews ><
In general, it's easier to review pull requests that address one problem at a time and that have specific details of the problems we want to fix.
For this PR, I understand that we should take out the Create Build step, but everything else seems like removing tests that we do want to run. If these tests are not passing, we should update the cookie cutter so they do pass. Maybe this PR should just be about removing the Create Build step?

rgraber · 2023-12-08T17:05:35Z

cookiecutter-xblock/{{cookiecutter.repo_name}}/.github/workflows/ci.yml

@@ -16,7 +16,7 @@ jobs:
      matrix:
        os: [ubuntu-20.04]
        python-version: ['3.8']
-        toxenv: [quality, docs, pii_check, django32, django40]


Can you explain a little more about why you are removing these? What errors were you seeing? In general, we'd prefer to correct the errors in the templates rather than remove the checks.

rgraber · 2023-12-08T17:07:04Z

cookiecutter-xblock/{{cookiecutter.repo_name}}/tox.ini

 match-dir = (?!migrations)

 [pytest]
 DJANGO_SETTINGS_MODULE = translation_settings
 addopts = --cov {{ cookiecutter.package_name }} --cov-report term-missing --cov-report xml
 norecursedirs = .* docs requirements site-packages

-[testenv]


Similar question, why are we removing these? What errors are they causing? Is it something we can fix rather than remove?

rgraber · 2023-12-08T17:10:37Z

cookiecutter-xblock/{{cookiecutter.repo_name}}/.github/workflows/ci.yml

@@ -31,13 +31,6 @@ jobs:
    - name: Install Dependencies
      run: pip install -r requirements/ci.txt

-    - name: Create Build


good catch!

rgraber · 2023-12-15T15:27:10Z

This work ended up dovetailing with some of my own so I took care of removing the build and the PII checks in #424 and #423. I'm going to close this PR, but thank you @Talha-Rizwan for catching that build thing and for all your work!

fix: ci workflow to pass the template xblock

d2aff99

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 28, 2023

itsjeyd added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. needs test run Author's first PR to this repository, awaiting test authorization from Axim labels Nov 29, 2023

rgraber mentioned this pull request Nov 29, 2023

Ci build fails for xblock #411

Closed

mphilbrick211 added needs test run Author's first PR to this repository, awaiting test authorization from Axim and removed needs test run Author's first PR to this repository, awaiting test authorization from Axim labels Nov 30, 2023

rgraber reviewed Dec 4, 2023

View reviewed changes

Co-authored-by: Rebecca Graber <[email protected]>

69c6946

Fix: replaced package name in tox.ini

Talha-Rizwan force-pushed the fix-xblock-workflow branch from 69ce950 to 69c6946 Compare December 5, 2023 13:33

fix: added back codecov

144d026

itsjeyd added needs test run Author's first PR to this repository, awaiting test authorization from Axim and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Dec 7, 2023

nedbat reviewed Dec 8, 2023

View reviewed changes

rgraber suggested changes Dec 8, 2023

View reviewed changes

rgraber reviewed Dec 8, 2023

View reviewed changes

rgraber added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 13, 2023

rgraber mentioned this pull request Dec 13, 2023

fix: remove pii_check from xblock cookiecutter #423

Merged

6 tasks

rgraber closed this Dec 15, 2023

itsjeyd added duplicate This issue or pull request already exists elsewhere and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. needs test run Author's first PR to this repository, awaiting test authorization from Axim labels Dec 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: xblock ci workflow #412

Fix: xblock ci workflow #412

Talha-Rizwan commented Nov 28, 2023

openedx-webhooks commented Nov 28, 2023

Talha-Rizwan commented Nov 28, 2023

itsjeyd commented Nov 29, 2023

Talha-Rizwan commented Nov 30, 2023

e0d commented Nov 30, 2023

Talha-Rizwan commented Nov 30, 2023

rgraber left a comment

rgraber Dec 4, 2023

Talha-Rizwan Dec 5, 2023

rgraber Dec 5, 2023

Talha-Rizwan Dec 5, 2023

rgraber Dec 4, 2023

Talha-Rizwan Dec 5, 2023

rgraber Dec 6, 2023

rgraber Dec 6, 2023

Talha-Rizwan Dec 7, 2023

e0d commented Dec 5, 2023

nedbat Dec 8, 2023

nedbat Dec 8, 2023

nedbat Dec 8, 2023

rgraber left a comment

rgraber Dec 8, 2023

rgraber Dec 8, 2023

rgraber Dec 8, 2023

rgraber commented Dec 15, 2023

Fix: xblock ci workflow #412

Fix: xblock ci workflow #412

Conversation

Talha-Rizwan commented Nov 28, 2023

openedx-webhooks commented Nov 28, 2023

Talha-Rizwan commented Nov 28, 2023

itsjeyd commented Nov 29, 2023

Talha-Rizwan commented Nov 30, 2023

e0d commented Nov 30, 2023

Talha-Rizwan commented Nov 30, 2023

rgraber 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e0d commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgraber 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

rgraber commented Dec 15, 2023