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

This removes the unnecessary variable INSTALL which has the same value as the recipe variable - (Solves- #437) #438

Merged
merged 1 commit into from Jul 16, 2019

Conversation

pranav1698
Copy link

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • There is a corresponding issue for this pull request.
  • My branch is up-to-date with the Upstream master branch.
  • The unit tests pass locally with my changes (provide a screenshot or link for test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Short description of what this resolves:

This solves the unexpected error Sorry, unexpected error: str expected, not list,
The INSTALL array was not required, the value of the array is same as the recipe array but with different names, I have made the changes equivalent to what we were doing previously
Check one of the meilix builds triggered by the webapp and notice that the recipe array is already sending the 3rd party softwares into the build server

Changes proposed in this pull request:

  • Removing the INSTALL variable

Fixes #437

Copy link
Member

@rpotter12 rpotter12 left a comment

Choose a reason for hiding this comment

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

@pranav1698 according to #121 we have to keep chrome also as an option.

@pranav1698
Copy link
Author

@tabesin @xeon-zolt do we need chrome even after we are already adding chromium

Copy link
Author

@pranav1698 pranav1698 left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@tabesin tabesin left a comment

Choose a reason for hiding this comment

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

What do you (plan to) do with the
INSTALL_xyz
then?

@pranav1698
Copy link
Author

pranav1698 commented Apr 3, 2019

@tabesin we can have the name of the inputs for the softwares same or different, value of the inputs will remain same
I thought that it will bring clarity if we specify the name of inputs with the value of inputs as we were doing before #435 is merged, we can still make the name of the inputs same

Copy link
Author

@pranav1698 pranav1698 left a comment

Choose a reason for hiding this comment

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

Copy link
Author

@pranav1698 pranav1698 left a comment

Choose a reason for hiding this comment

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

@tabesin please review, this is open for a long time

@tabesin tabesin requested a review from rpotter12 April 16, 2019 07:50
Copy link
Member

@rpotter12 rpotter12 left a comment

Choose a reason for hiding this comment

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

@pranav1698 wait for some time I will create a PR for full working 3rd party software installation. I'm busy in my semester final exams.
You can change this PR by removing the old way of getting the response of checkbox and please don't remove the chrome checkbox (refer #121)

@xeon-zolt
Copy link
Member

chromium and chrome are the same things

@rpotter12
Copy link
Member

Through meilix-generator we are providing an option for users to choose his/her own browser, so providing an option for chrome also should not be a problem.

@tabesin tabesin merged commit 3ebe4cd into fossasia:master Jul 16, 2019
@rpotter12
Copy link
Member

@tabesin this pull request should not be merged because the changes are already made in meilix repository with respect to old changes #435 . This pull request will break the installation of 3rd party software which are selected by checkbox.

@rpotter12
Copy link
Member

@tabesin @xeon-zolt @meets2tarun please review it again

@meets2tarun
Copy link
Member

ok, lets have a look,

thanks for the notification @rpotter12

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

Successfully merging this pull request may close these issues.

Unexpected error on clicking the build button
5 participants