Skip to content

Conversation

@jasp00
Copy link
Member

@jasp00 jasp00 commented Sep 21, 2016

This adds an hourly cron job to maintain the repository lmms. Is an access to GitHub set up already? A commit preparing the affected files should be done first.

Copy link
Member

@tresf tresf left a comment

Choose a reason for hiding this comment

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

Hourly may be overkill, don't you think?

@jasp00
Copy link
Member Author

jasp00 commented Sep 21, 2016

Hourly may be overkill, don't you think?

It is only a git pull and the scripts are quick. I could avoid the scripts though, if there were no errors.

@jasp00
Copy link
Member Author

jasp00 commented Sep 21, 2016

I could avoid the scripts though, if there were no errors.

Done.

@tresf
Copy link
Member

tresf commented Jan 28, 2017

  • Current cron file naming convention looks like this: update_community_feeds. Please replace dashes with underscores.
  • The file name lmms-tasks should be more descriptive, such as git_maintenance_tasks
  • Although the intent of maintenance-tasks-done seems to be to make sure it runs at least once, the code suggests it's not needed; it'll hit else on the if [ -e lmms ] block.
  • To be safe, git clone [...] should explicitly specify a branch. I recommend master.
  • Already up-to-date. should set terminal to English to make sure this parses the English output (e.g. LC_ALL=C)

I like the thought that went into the cron/tasks structure and I don't mean to be overly critical, but I feel this could all be in one script.

For example... here's the entire task in 34 lines with comments. I haven't tested the below code but in addition to the above points... When run interactively, it should be apparent what it's doing for support purposes.

#!/bin/sh

# Exit on any non-zero return codes; force en_US locale
set -e && LC_ALL=C
mkdir -p ~/lmms-repo && cd ~/lmms-repo

if [ -e lmms ]; then
	cd lmms && PULLRES=$(git pull)
	if [ "$PULLRES" = "Already up-to-date." ]; then
		exit
	fi
else
	git clone -b master https://github.com/LMMS/lmms && cd lmms
fi

# Update CONTRIBUTORS
git shortlog -sne | cut -c 8- > doc/CONTRIBUTORS

# Update PROJECT_YEAR 
YEAR=$(date -u +%Y)
while true; do
	if [ "$(git log -n 1 --since=$YEAR-01-01T00:00:00Z)" ]; then
		break
	fi
	YEAR=$((YEAR - 1))
done

sed -e "/^SET(PROJECT_YEAR / c \SET(PROJECT_YEAR $YEAR)" -i CMakeLists.txt

# Commit changes, if any
if [ "$(git diff)" ]; then
	git add -u && git commit -m "Maintenance tasks"
	git push origin master
fi

@tresf
Copy link
Member

tresf commented Jan 28, 2017

Is an access to GitHub set up already?

No, not yet. lmmsservice is our GitHub service account. I'd be happy to set it up with a pointer in the right direction. Alternately, we can add your public key to the server if you prefer to test it all out yourself first.

@jasp00
Copy link
Member Author

jasp00 commented Feb 3, 2017

  • Although the intent of maintenance-tasks-done seems to be to make sure it runs at least once, the code suggests it's not needed

It is needed to run the tasks at most once, otherwise the tasks would run each hour.

I feel this could all be in one script.

The point is to easily add, enable and debug tasks; they are like standalone plug-ins. Each task should have its own commit actually. More tasks are expected to be developed.

we can add your public key to the server

This is an option, but not necessary.

I'd be happy to set it up with a pointer in the right direction.

Option 1, with SSH:

  1. Generate 4096-bit RSA SSH keys.
  2. Upload the public key to GitHub.
  3. Configure ~/.ssh/config.

Option 2, with password:

  1. Configure ~/.netrc.

@tresf
Copy link
Member

tresf commented Feb 3, 2017

It is needed to run the tasks at most once, otherwise the tasks would run each hour.

If it only commits changes, I don't see how this statement is valid.

The point is to easily add, enable and debug tasks; they are like standalone plug-ins. Each task should have its own commit actually.

That's an opinion. As the site maintainer, I won't merge it in this condition. If another maintainer disagrees you can work with them.

More tasks are expected to be developed.

This is speculative.

Option 1, with SSH:

Thanks. I'll post back to this thread once setup.

Break single commit into task commits
@jasp00
Copy link
Member Author

jasp00 commented Feb 4, 2017

The goal is scalability.

otherwise the tasks would run each hour.

If it only commits changes, I don't see how this statement is valid.

The tasks would not make any commit, but they would still run (do you remember?).

The point is to easily add, enable and debug tasks;

That's an opinion.

Why would you not want a system that is based on run-parts, which is easy to debug single tasks, to disable with a file rename, to expand with independent executables, etc.? Why would you want a system that is not scalable?

More tasks are expected to be developed.

This is speculative.

I mentioned some possible tasks: remove executable bits, optimize PNG images, format code, etc. Do you want me to add another task?

@tresf
Copy link
Member

tresf commented Feb 4, 2017

The tasks would not make any commit, but they would still run (do you remember?).

What you're referencing is the question if hourly was overkill. Your argument I felt was valid.

jasp00 wrote: "It is only a git pull and the scripts are quick. I could avoid the scripts though, if there were no errors."

Still, I don't like magic lock files. They're bad design as they require manual cleanup when things fail. Last, I don't even understand the purpose of this one. I can't see a logical scenario when it's warranted.

I mentioned some possible tasks: remove executable bits, optimize PNG images, format code, etc. Do you want me to add another task?

Very good points. Sorry for the delay on the SSH push config. Will set that up ASAP.

@jasp00
Copy link
Member Author

jasp00 commented Feb 4, 2017

Still, I don't like magic lock files. They're bad design as they require manual cleanup when things fail.

There is no cleanup, lock files do not need to be removed. These locks are released when the process finishes. AFAIK, this is the best way to have only one instance per service.

Last, I don't even understand the purpose of this one. I can't see a logical scenario when it's warranted.

It is defensive coding. The case is when the process lasts more than an hour somehow. This may happen if networking goes slow, if the server is heavy loaded, if a task is deadlocked, if we include 60 one-minute tasks, etc. If this happens, we do not want a second instance to interfere with the first one.

I should add a protection against merge conflicts.

@jasp00
Copy link
Member Author

jasp00 commented Feb 4, 2017

I should add a protection against merge conflicts.

In the future, it is very unlikely right now.

@tresf
Copy link
Member

tresf commented Feb 5, 2017

I should add a protection against merge conflicts.

In the future, it is very unlikely right now.

As is a 1-hour-long merge.

@jasp00
Copy link
Member Author

jasp00 commented Feb 5, 2017

As is a 1-hour-long merge.

1-hour-long process actually; I know it is unlikely. I would implement a protection against merge conflicts, but I have more pressing issues right now. The point is I would not reject an improvement.

@tresf
Copy link
Member

tresf commented Feb 7, 2017

@jasp00 SSH authentication is complete.

  • Pushes will authenticate automatically; If for some reason they don't, we can prepend ssh-agent to the git push command.
  • Pushes must go to ssh instead of the default HTTP push.
    # example
    git remote set-url origin [email protected]:lmms/lmms.git
  • Here is a preview of what the push will look like: tresf/lmms@c7f025f

@jasp00
Copy link
Member Author

jasp00 commented Feb 13, 2017

We should test with a branch first, then switch to master, do you agree?

@tresf
Copy link
Member

tresf commented Feb 13, 2017

We should test with a branch first, then switch to master, do you agree?

Sure.

Test with another branch
@jasp00
Copy link
Member Author

jasp00 commented Feb 13, 2017

Ready.

@tresf tresf merged commit cb87cc0 into LMMS:master Feb 13, 2017
@tresf
Copy link
Member

tresf commented Feb 13, 2017

Should kick off in 28 minutes...

@jasp00 why are the scripts in /dev and /tasks and not /cron ?

@tresf
Copy link
Member

tresf commented Feb 13, 2017

Answering my own question, dev was already there. I'm still confused as to the need for the new root directory /tasks. Seem like /dev could have been the home for tasks.

@tresf
Copy link
Member

tresf commented Feb 13, 2017

Ok... @lukas-w put the other script in /dev for a different purpose -- to help him test locally. We need to move stuff around. /cron can house everything and /dev should go back to just developer helpers/tools.

@jasp00 jasp00 deleted the tasks-lmms branch February 15, 2017 05:25
@jasp00
Copy link
Member Author

jasp00 commented Feb 15, 2017

I'm still confused as to the need for the new root directory /tasks.

I chose /tasks to be independent of the trigger, either cron job or webhook.

We need to move stuff around.

Sure.

There are no commits in the test branch. Are you sure cron is running? Should I prepend ssh-agent?

@tresf
Copy link
Member

tresf commented Feb 15, 2017

Cron is running, there was just an authentication problem which has been fixed d2d6704.

Note, the task was run interactively for testing purposes; the commits (LMMS/lmms@b990b18, LMMS/lmms@bf0e650), were done with the interactive shell and on initial clone (haven't tested pull) but initial tests pass.

if [ -e lmms ]
then
cd lmms
PULLRES=$(git pull)
Copy link
Member

Choose a reason for hiding this comment

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

@jasp00 should we consider appending git checkout $BRANCH to force us back onto master once ready?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, #214.

@jasp00 jasp00 mentioned this pull request Feb 16, 2017
@tresf
Copy link
Member

tresf commented Feb 16, 2017

I can't find your most recent comments about the job failing but interactively tests pass, then when I scheduled the hourly to run now, it reverted the branch to the past somehow. https://github.com/LMMS/lmms/blob/maintenance-test/doc/CONTRIBUTORS

@tresf
Copy link
Member

tresf commented Feb 16, 2017

Ok... @jasp00 I think we've hit a nuance with git add.

http://stackoverflow.com/questions/348170/how-to-undo-git-add-before-commit

I can reproduce the flapping behavior of HEAD and reset the file. We can either try moving git add inside the conditional block, or try git reset <file> to remove it although there's a very highly up-voted answer that instead says we should use git rm --cached <file>.

@jasp00
Copy link
Member Author

jasp00 commented Feb 16, 2017

Ok... @jasp00 I think we've hit a nuance with git add.

I do not think so, git add works as intended. See #215 (comment).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants