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.

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

Update wiki to reflect latest Vagrant practice with DSpace 6 #659

Open
alawvt opened this issue Dec 3, 2019 · 2 comments
Open

Update wiki to reflect latest Vagrant practice with DSpace 6 #659

alawvt opened this issue Dec 3, 2019 · 2 comments

Comments

@alawvt
Copy link
Contributor

alawvt commented Dec 3, 2019

To reflect changes in our vtlibans Vagrant procedure, we need to update our procedures described in pages 3-5 in the GitHub wiki, https://github.com/VTUL/vtechworks/wiki/05.-Pushing-changes-to-Github. I have already made some updates to the vtlibans README, https://code.vt.edu/it-services/vtlibans_dspace/blob/master/README.md.

We want the instructions in the vtlibans README and the GitHub wiki to coordinate well. The instructions in the GitHub wiki describe our old method of making changes on the LDE and then committing one commit at the end to GitHub. With our current method, we sometimes have to push many commits while working on a branch. We could squash everything at the end to one commit, but my understanding is that is not best practice after smaller commits are pushed to a public repository. - @ALaw

@alawvt
Copy link
Contributor Author

alawvt commented Dec 3, 2019

I couldn't find the bit in the wiki about making a single commit, but that's usually considered a good practice, with the context that changes should be small and that one commit per change makes it easy to see the scope of a change.

I used the "squash" feature that's now built into GitHub to squash some of Kimberli's recent commits into one commit before merging into the main branch. That's usually considered a good practice, too, but there's a lot of variation from team to team.

I think that squashing may be considered a bad practice when squashing commits that have already been merged into a master branch, or squashing commits on feature branches that multiple people are working on. Then the history that people have on their local machine won't match.

Usually it's encouraged that people squash related commits for one small change into a single commit before pushing to GitHub. That avoids any problems related to squashing after pushing to public repositories, and still gives the benefits of squashing.

I've purposely broken the single commit per feature change practice at times. (Actually, perhaps too often with VTechWorks). Here are some examples:

  • When customizing DSpace modules for VTechWorks via the overlay method, I usually make one commit that's a copy of the original file in the overlay location, and then a second commit - the changed file - that makes it easier to see our customizations. If this were a single commit, it would just look like a new file had been created in the overlay, and it's more work to see the changes.
  • When working on some customizations for VTechWorks, like when we first did the upgrade when Amanda was here, I initially left lots of really small commits in the vt_5_x_dev branch just to sort of leave a trail for others to follow in case they needed to do theme customizations. I probably overdid it a bit (but we were also working really incrementally at the time, sometimes looking at changes frequently).

I guess it depends on how you want the git history to look - do you want to see exactly which code changed for which feature change? Or do you want to look at bits and pieces separately?

Generally squashing into a single commit before merging to the main branch is considered good, as long as the final commit applies to only one change, and it's often considered good practice for a developer to squash commits themselves before pushing a branch to another location. But I'd still advocate for any DSpace overlay customizations to have separate commits for the copy of the file to the new location, and then another commit that shows our local changes. - @keithgee

@alawvt
Copy link
Contributor Author

alawvt commented Dec 3, 2019

Thank you for your thoughtful reply to my query. I agree with you on all points. You make a good point that squashing is good for a feature branch but not the main branch. I will try to incorporate all these practices as I update the GitHub wiki to reflect our Vagrant procedure. We can review this at the DevOps meeting on Thursday. - @alawvt

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

No branches or pull requests

1 participant