Skip to content

Begin the commit process with index reset #3486

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

garryshtern
Copy link
Contributor

Pre-Request Checklist

  • Passes rubocop code analysis (try rubocop --auto-correct)
  • Tests added or adapted (try rake test)
  • Changes are reflected in the documentation
  • User-visible changes appended to CHANGELOG.md

Description

outputs/git: Begin the commit process with the index reset to the tree of the HEAD commit.

This prevents the unintentional re-inclusion of files in the repo that are not referenced by the HEAD commit or the device being updated

This should close issue #1805

outputs/git: Begin the commit process with the index reset to the tree of the HEAD commit.

This prevents the unintentional re-inclusion of files in the repo that are not referenced by the HEAD commit or the device being updated
@robertcheramy
Copy link
Collaborator

I disagree on this.

Re-reading the tree costs computing ressources, see the comment above the function. Rereading at each commit seems a waste of ressources to me.

The only reason why the index becomes out of sync is the user tempering with the git repository of oxidized. This is documented at https://github.com/ytti/oxidized/blob/master/docs/Troubleshooting.md#oxidized-ignores-the-changes-i-made-to-its-git-repository

If we want to get rid of the index on disk, so that it becomes more intuitive for the user (stop oxidized, fiddle with git, start oxidized), we could cache the index with the help of a cache method. Or we keep the index on disk and only track the information that the index wasn't re-read since last start of oxidized.
Last time I worked on this part of oxidized, I decided that the documentation above is enough and the effort was not worth it. But I may be wrong.

#1805 is about deleting devices configuration from when they are not listed in source anymore., so this PR does not solve the issue.

@garryshtern
Copy link
Contributor Author

That’s not the only way for index to be out of sync. You are assuming that there is only a single instance of oxidized that is setup, which isn’t always the case. If you have two parallel deployments with the same repo backend, then a change in one will break the other one, without an index reread.

@robertcheramy
Copy link
Collaborator

I would not recommend using the same repo for two oxidized processes. You will run in unexplainable sync issues, even if you re-read the index each time: if both oxidized make a commit at the same time, only one would persist, because they both start with the same index.

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