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

New release #5

Open
rafaelbco opened this issue Aug 17, 2017 · 21 comments
Open

New release #5

rafaelbco opened this issue Aug 17, 2017 · 21 comments
Assignees

Comments

@rafaelbco
Copy link
Contributor

@vangheem Can you make a new release or give me permission on PyPI to do so? My username is rafaelbco there.

@vangheem
Copy link
Member

You'll want to direct these questions to @zombified or @tkimnguyen

@tkimnguyen tkimnguyen self-assigned this Aug 17, 2017
@gforcada
Copy link
Contributor

@tkimnguyen you can also give collective and/or plone pypi users rights to do the releases as well if you wish.

@tkimnguyen
Copy link
Member

Thank you both @rafaelbco and @gforcada for the Plone 5 compatibility and other changes. I am checking with my colleagues and will get back to you very shortly.

@tkimnguyen
Copy link
Member

Re: the other (non Plone-5 compatibility) changes: as you can imagine, we do use this package, and we designed it to be as simple as possible and to minimize the attack surface. Giving any additional information to potential attackers is generally something to avoid. Adding an API and a view for managing the settings also goes counter to that. I apologize for this misunderstanding and wish we could have communicated this before you went to all this trouble. We understand that you could want to fork this package, and if you do, we would appreciate it if you did not retain the wildcard namespace. On the other hand, we would love for this package (minus the increased attack surface) to be included in core. Can we find a way to make this work?

@rafaelbco
Copy link
Contributor Author

It is not clear to me what you want to do. Do you want to revert the changes? Do you want to keep the changes in the repo but not make a new release?

Just to add some background information:

  • I created a repo in the collective namespace called collective.readonlymode. Right after that @vangheem told me via an issue that wildcard.lockdown maybe would address the same needs I was trying to address, which I confirmed.
  • @gforcada showed interest in the discussion that took place in the collective.readonlymode issue.
  • The collective.readonlymode repo was deleted.
  • I then told @vangheem, who was the author of all the commits in this repo at the time, that I wanted to implement the Web API and the status message feature. He said "I don't use this package anymore. Any development done is welcome. Have at it."
  • So I implemented the changes and assigned @gforcada and @vangheem as reviewers.
  • The review process went on and my pul request was merged, introducing the Web API.

@rafaelbco
Copy link
Contributor Author

as you can imagine, we do use this package

I didn't imagine, because the only info I had was from @vangheem (who was the author of all commits) and he mentioned he didn't use the package anymore.

and we designed it to be as simple as possible and to minimize the attack surface. Giving any additional information to potential attackers is generally something to avoid. Adding an API and a view for managing the settings also goes counter to that.

Why "also"? What is the other feature that give additional information to potential attackers?

@tkimnguyen
Copy link
Member

Just to answer your easy questions right away:

  • Nathan personally no longer uses these packages but we at Wildcard (his former employer) still do. Sorry, he and I both assumed you knew about Wildcard and our security-conscious clients.
  • The "also" refers to the status message about how the site is in read only mode.
  • I'm not sure what we want to do next; I was looking for your feedback and suggestions, and am trying not to piss you off :)

@rafaelbco
Copy link
Contributor Author

rafaelbco commented Aug 18, 2017

Since you're trying to not me piss me off (thanks!) I'll try to convince you to approve the new features. If you're ok with the new features from a technical point of view then the problem disappears.

The status message is optional, and it's disabled by default. When it's enabled it's shown only to logged in users. So I see no big issue here.

Regarding @@manage-lockdown: there's already a view that provides exactly the same functions. It's called @@lockdown-settings. This is the view used in the control panel. Everything I can do with the new view I can also do with the existing one. The only difference is that the new view is more curl friendly. Would it stop a hacker? I don't think so. Both are protected by the same permission: cmf.ManagePortal.

@gforcada
Copy link
Contributor

@tkimnguyen if you don't want the changes you can also pin the current version and stick to it.

If new development should happen from your side, either double review what will be in master or create a branch out of the last tag you are interested on.

If the package is on collective I always assume that is open to everyone to contribute, improve and adapt to their needs as well. Right?

@rafaelbco
Copy link
Contributor Author

rafaelbco commented Aug 18, 2017 via email

@tkimnguyen
Copy link
Member

Thanks for your patience and for explaining your reasoning! I think we should go with the idea of changing version numbers to at least 1.1 so that we at Wildcard can pin to the current feature set and you are free to continue developing. Indeed, this is in the collective so that everyone can build on top of it and add to it. Please let me know when you want a release made.

@tkimnguyen
Copy link
Member

tkimnguyen commented Aug 22, 2017

Could we maintain separate branches for this? We would prefer to leave master as the 'minimalist' branch with the current feature set, and maybe your new features would be added to the 'comprehensive' branch? If your versions are something like 1.1-comprehensive it would be easy to distinguish.

@rafaelbco
Copy link
Contributor Author

I'm starting to think that the best solution would be a fork. I would create collective.lockdown and life would go on.

Forks exists, it's no big deal IMO.

I'd like to hear @gforcada opinion on this.

@gforcada
Copy link
Contributor

If wildcard does not plan to further develop. As they prefer a minimalist approach, I would rather keep a branch for them (or anyone wanting only the minimal set) and keep master for "active" development, that's what any outsider to this discussion would think about.

As for branches, just like any plone core package, a 1.x branch can be left for the minimalist approach and a 2.x for the new ongoing development.

Sounds that good?

@rafaelbco
Copy link
Contributor Author

rafaelbco commented Aug 23, 2017

I agree.

And it's technically simple. Just release what's in master now as 2.0 and cut a 1.x branch before unwanted features were added.

@tkimnguyen
Copy link
Member

I was discussing a bit further with @zombified ... could we have the master branch be the 'minimalist' one? I understood it might be easier to cherry pick commits to the "extras" branch into master than the other way around.

@rafaelbco
Copy link
Contributor Author

This is the same suggestion made before and I still prefer the approach suggested by @gforcada.

Just to be clear, what I think has to be done is:

  • Release what's in master now as version 2.0.
  • Cut a 1.x branch before unwanted features were added.

This is standard procedure when compatibility breaking features are added to a software package, at least in the Zope world.

I understood it might be easier to cherry pick commits to the "extras" branch into master than the other way around.

I fail to understand why. The master branch is not special regarding the "git cherry-pick" command.

@tkimnguyen
Copy link
Member

The reason why we prefer to have master be minimalist is that we want the default add-on to be locked down. Maybe having someone add wildcard.lockdown[extras] would be easy to understand.

@rafaelbco
Copy link
Contributor Author

I understand the reason, but I disagree. But it's just my opinion. I'm not the maintainer.

Maybe having someone add wildcard.lockdown[extras] would be easy to understand.

As far as I know the [extras] thing is just to declare extra dependencies. Since the new features do not depend on any extra distribution, this would not work.

@gforcada
Copy link
Contributor

gforcada commented Apr 11, 2018

@tkimnguyen @rafaelbco sorry to bother you almost a year later :-) can we get an agreement somehow?

Let's see if you both like my plan:

  • last version on pypi is 1.0a1, can we make a final 1.0 without the changes introduced (i.e. just re-releasing 1.0a1) this release together with the usual tag 1.0.0 and a 1.x branch will be created and control over it will be managed by @tkimnguyen and wildcard folks
  • a new release 2.0 (does it need alpha/beta @rafaelbco ?) is made out of current master, i.e. with the changes introduced by @rafaelbco and the usual 2.0.0 tag is created
  • for extra clarification, we could update the README on both branches before the 1.0 and 2.0 releases to explain this situation: 1.x releases are as minimalist as possible, 2.x and future releases might add more functionality that if you are so much concerned about security should double and triple check.

I hope that this is reasonable for you all. I tried to follow best practices and still make it clear which release is what.

One last thing, that goes to @tkimnguyen would you be open to allow more maintainers in pypi or would you still prefer to have control over that and keep adding tickets here in the issue tracker whenever a new release is wished for?

@rafaelbco
Copy link
Contributor Author

I'm ok with the plan suggested by @gforcada

I have not used the version with my changes in production yet, so I think the 2.0 release should be marked as beta.

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

4 participants