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

Revert "Fix github actions for non-release tags" #236

Merged
merged 2 commits into from Mar 19, 2020

Conversation

shyamd
Copy link
Contributor

@shyamd shyamd commented Mar 18, 2020

Reverts #235

@codecov
Copy link

codecov bot commented Mar 18, 2020

Codecov Report

Merging #236 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #236   +/-   ##
=======================================
  Coverage   87.33%   87.33%           
=======================================
  Files          43       43           
  Lines        1943     1943           
=======================================
  Hits         1697     1697           
  Misses        246      246           
Flag Coverage Δ
#unittests 87.33% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55c84c3...d2e1fba. Read the comment docs.

@CasperWA
Copy link
Member

CasperWA commented Mar 18, 2020

Cool.
So, I have been testing some stuff on my fork and have come to the following conclusion(s):

  • When one creates a tag, it will become part of that repository's list of tags, no matter the commit (in the specific repository) it is made to point to. Since we've been working on Materials-Consortia branches for the release PRs, the tag is created for the Materials-Consortia repository, even when pushing the tag in a PR.
    In the end, this means, even when the PR is merged, there are no tag pushes from whatever branch to master, since the tag is simply a "symlink" (of sorts) to a commit SHA. That the commit has now become part of master makes no difference. Even if the commit doesn't exist in any branch anymore, the tag will persist.
  • Setting a workflow to run on "push to master", will result in a GitHub context containing the commits in the PR, which are now in master. There is no reference or information about linked tags to any commits - and it also wouldn't make sense, since a tag has no influence on the commits in a branch.

So - we should decide on a workflow that will work for us.

  1. Either we keep the workflow running on "push specific kind of tag" and make sure to only create and push these tags on commits already in master, so as to not publish a tag pointing to a pre-mature commit. I.e., one creates a PR to update the code with the new version, has it merged, and immediately creates a tag and pushes it, starting the workflow (as has been the way to do it up to now).
  2. Or we decide to instead rely on creating GitHub releases, which will in turn start the Actions workflow to publish on PyPI as well. This means nothing will happen when creating and pushing tags, only when publishing/releasing a tag on GitHub. (EDIT: Chosen option)

Personally, I don't mind which it will be.

@ml-evs
Copy link
Member

ml-evs commented Mar 18, 2020

I much prefer 2, as a great many people have write access to this repo, and just pushing a versioned tag to their own branch will lead to a PyPI release, which potentially runs on many servers, who are potentially automatically updating dependencies... etc.

@shyamd
Copy link
Contributor Author

shyamd commented Mar 18, 2020

I also vote for the second option. There needs to be step that initiated only for a release so it doesn't accidentally happen.

@CasperWA
Copy link
Member

I much prefer 2, as a great many people have write access to this repo, and just pushing a versioned tag to their own branch will lead to a PyPI release, which potentially runs on many servers, who are potentially automatically updating dependencies... etc.

If they first change the if statement in the workflow, yes.

@CasperWA
Copy link
Member

All right! I'll push the solution for option number 2 to this PR, after I have tested it a bit in my fork first 👍

@ml-evs
Copy link
Member

ml-evs commented Mar 18, 2020

I much prefer 2, as a great many people have write access to this repo, and just pushing a versioned tag to their own branch will lead to a PyPI release, which potentially runs on many servers, who are potentially automatically updating dependencies... etc.

If they first change the if statement in the workflow, yes.

What do you mean? I managed to make a release off my own personal branch without changing anything

@CasperWA
Copy link
Member

I much prefer 2, as a great many people have write access to this repo, and just pushing a versioned tag to their own branch will lead to a PyPI release, which potentially runs on many servers, who are potentially automatically updating dependencies... etc.

If they first change the if statement in the workflow, yes.

What do you mean? I managed to make a release off my own personal branch without changing anything

Ah! Hmm... yeah okay - from a PR then? I guess the if statement is not that much of a show stopper in the end then.

@CasperWA
Copy link
Member

On another note, related to my "conclusions" above. It should be considered bad practice to create and push a tag on a PR commit, before the PR is merged. Since, if the PR is squashed and merged or rebased and merged, the tag will persist, but point to a non-historical/-existing commit.
If one has done this, however, it can still be okay, but only if the PR is merged from the commandline using git merge --ff-only - or merging including a merge commit. However, then the master will be already 1 commit ahead of the latest release without having actually added an extra real commit ....

@CasperWA
Copy link
Member

CasperWA commented Mar 18, 2020

After merging this, we can test it by releasing the v0.7.1 tag on GitHub? Unless we want to actually set it to point to this PR instead?

Edit: I have just deleted the test release for v0.7.1. I think I would suggest we also remove the v0.7.1 tag and recreate it for the latest commit here?

shyamd and others added 2 commits March 19, 2020 15:42
A python script checks the validity of the release metadata, i.e., if
the tag matches "vMAJOR.MINOR.PATCH", extras after PATCH are not
allowed, and the version _must_ be prefixed by "v".
Furthermore, it checks that the released tag points to a commit in the
`master` branch.

If nothing of these things are true, it will stop the publish on PyPI
workflow and one should delete the release from GitHub.
@CasperWA
Copy link
Member

@shyamd do you approve my commit? Then I'll approve on your behalf and we can merge :)

@shyamd
Copy link
Contributor Author

shyamd commented Mar 19, 2020

I approve of this PR.

Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Approved on behalf of @shyamd

@CasperWA CasperWA merged commit d2e1fba into master Mar 19, 2020
@CasperWA CasperWA deleted the revert-235-fix-rls-on-tag branch March 19, 2020 16:22
@ltalirz ltalirz temporarily deployed to optimade March 19, 2020 16:25 Inactive
@CasperWA CasperWA mentioned this pull request Apr 22, 2020
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.

None yet

4 participants