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.

Sign up for GitHub

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

Jump to bottom

Add github action to check the nix build #1653

Merged
merged 11 commits into from
May 7, 2024
Merged

Add github action to check the nix build #1653

merged 11 commits into from
May 7, 2024

Conversation

lucc
Copy link
Collaborator

@lucc lucc commented May 2, 2024 •
edited
Loading

This adds CI jobs to run the nix build of alot and also a second nix target with a ci job to build the sphinx docs.

Some test are fixed or removed, see commit messages. The last question about the fixed test is #1653 (comment)

The plan behind this is that we have some CI that is green until the pip/poetry installation of alot is fixed again and our other CI jobs can be trusted. And also to check that the nix code works correctly because I personally rely on it.

@pazz
Copy link
Owner

pazz commented May 3, 2024

Is this work in progress or ready to merge? I trust you know what you are doing wrt github actions as I don't :)

@lucc lucc marked this pull request as draft May 3, 2024 13:28
@lucc
Copy link
Collaborator Author

lucc commented May 3, 2024

Sorry this is still WIP as some tests are still failing in the nix sandbox.

@pazz
Copy link
Owner

pazz commented May 3, 2024 via email

@lucc
Copy link
Collaborator Author

lucc commented May 5, 2024

@pazz & @guludo what do you think about 1359aba? I just removed the broken test, it would also be possible to add some validation code to the read_notmuch_config function to throw that exception again. We are only using the following values from the notmuch config:

  • maildir.synchronize_flags, bool as the string "true" or "false" in the notmuch config
  • database.path, a path as a string in the notmuch config
  • search.exclude_tags, a list of strings as a ";"-separated string in the notmuch config

so we could also parse these into special values inside read_notmuch_config.

@lucc lucc force-pushed the gh-flake branch 2 times, most recently from 130f1de to a6ac98c Compare May 5, 2024 13:09
@lucc lucc marked this pull request as ready for review May 6, 2024 07:40
@guludo
Copy link
Contributor

guludo commented May 6, 2024

@pazz & @guludo what do you think about 1359aba?

Looks good to me.

I just removed the broken test, it would also be possible to add some validation code to the read_notmuch_config function to throw that exception again.

That would make sense. Ideally, notmuch config would raise an error, since the documentation says that valid values are either true or false, but that does not seem to be the case.

lucc added 6 commits May 7, 2024 07:03
@lucc
Move validation of notmuch config values to read_notmuch_config
7a82785 
The related test for the config validation of "synchronize_flags" in
tests/settings/test_manager.py was broken since 023cf16, where the
parsing of the notmuch config file was migrated from the python
configobj based parsing function to loading the notmuch settings
including defaults from the notmuch CLI.
@lucc
Mock notmuch config file to fix test test_save_named_query
ef0b5e8
@lucc
Patch test that breaks in the nix sandbox
99163fb
@lucc
Remove env parameter from call_cmd_async
918dbd7 
The parameter is never used in alot but we have a failing test for it.
Instead of fixing the test the parameter is removed.
@lucc
Add nix derivation to build the alot html docs and man page
c4216f1
@lucc
Build the docs with nix in ci
a107e84
@lucc
Deactivate broken CI workflows
89fd7ce 
These workflows try to install the gpg dependency of alot via pip but
since c1137ea it is required as > 1.10.0 and such a version is not
currently available on PyPI.

See #1630 for context.  When the dependency problem is resolved these
workflows can be activated again.
@lucc
Copy link
Collaborator Author

lucc commented May 7, 2024

From my side this is ready for review (and merge).

@pazz is it ok to ignore the codeclimate check?

pazz
pazz approved these changes May 7, 2024
View reviewed changes
Copy link
Owner

@pazz pazz left a comment

Choose a reason for hiding this comment

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

I do not have experience with nix, flake nor github actions but the changes look good and to me, especially the work around the gpg issue. If this fixes the current problems with the CI pipeline then let's go for it! Many thanks!

alot/db/manager.py
@@ -72,7 +70,7 @@ def flush(self):
raise DatabaseROError()
if self.writequeue:
# read notmuch's config regarding imap flag synchronization
sync = settings.get_notmuch_setting('maildir', 'synchronize_flags') == 'true'
sync = settings.get_notmuch_setting('maildir', 'synchronize_flags')
Copy link
Owner

Choose a reason for hiding this comment

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

lol

@lucc lucc merged commit c6f3c6b into master May 7, 2024
7 of 8 checks passed
@delete-merged-branch delete-merged-branch bot deleted the gh-flake branch May 7, 2024 14:05
@chenrui333 chenrui333 mentioned this pull request Aug 26, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pazz pazz pazz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lucc @pazz @guludo